-
Notifications
You must be signed in to change notification settings - Fork 1.8k
chore(config): Fix InlineSingleUseReferencesVisitor
failing merges
#23207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The above visitor is intended to inline schemas in the references section for which there is only a single reference. This visitor is, however, unable to detect some cases where there are in fact multiple references, ending up inlining the schema multiple times. This has led to bloat in generated schemas. In the core Vector schema, this affects 121 structures, shaving 844 lines off of the generated schema.
Datadog ReportBranch report: ✅ 0 Failed, 7 Passed, 0 Skipped, 25.26s Total Time |
There's a failing check: https://github.com/vectordotdev/vector/actions/runs/15645373779/job/44081780346?pr=23207 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors how InlineSingleUseReferencesVisitor
counts schema references by replacing a HashSet
of parent references with a simple occurrence count (usize
), and adds tests for various reference contexts (arrays, composition keywords, additional/pattern properties).
- Switched
OccurrenceVisitor
’soccurrence_map
to track counts instead of sets. - Removed parent-scope deduplication logic and corresponding helper method.
- Added new tests to cover array items,
oneOf
/anyOf
/allOf
,additionalProperties
, andpatternProperties
.
Comments suppressed due to low confidence (3)
lib/vector-config/src/schema/visitors/inline_single.rs:143
- [nitpick] Consider renaming
occurrence_map
to something likeoccurrence_count_map
or adding a doc comment, since it now stores ausize
count rather than a set of references, which improves clarity.
struct OccurrenceVisitor {
lib/vector-config/src/schema/visitors/inline_single.rs:318
- The
single_ref_multiple_usages
test is still expected to fail (as noted in the TODO) but its#[ignore]
attribute was removed, causing CI to break. Please either restore#[ignore]
or implement the required logic to satisfy this test.
#[test]
lib/vector-config/src/schema/visitors/inline_single.rs:318
- The
multiple_refs_mixed_usages
test is still marked as a known issue in the TODO but is no longer ignored; this will cause the test suite to fail. Please re-add#[ignore]
or update the logic to pass this test.
#[test]
@@ -218,12 +218,8 @@ base: components: sources: static_metrics: configuration: { | |||
type: float: {} | |||
} | |||
bins: { | |||
description: """ | |||
A split representation of sketch bins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that this bit was deleted.
Summary
The above visitor is intended to inline schemas in the references section for which there is only a single reference. This visitor is, however, unable to detect some cases where there are in fact multiple references, ending up inlining the schema multiple times. This has led to bloat in generated schemas.
In the core Vector schema, this affects 121 structures, shaving 844 lines off of the generated schema.
I've labelled this as a
chore
as it doesn't actually make any functional change to the result.Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
Notes
@vectordotdev/vector
to reach out to us regarding this PR.pre-push
hook, please see this template.cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo nextest run --workspace
(alternatively, you can runcargo test --all
)./scripts/check_changelog_fragments.sh
git merge origin master
andgit push
.Cargo.lock
), pleaserun
cargo vdev build licenses
to regenerate the license inventory and commit the changes (if any). More details here.References