test: correct feature gating of two datafusion-common tests#23044
Merged
neilconway merged 2 commits intoJun 19, 2026
Conversation
`test_create_hashes_with_quality_hash_state` asserts that `create_hashes` reproduces a specific high-quality hash distribution. Under the `force_hash_collisions` feature `create_hashes` is replaced by a stub that writes all-zero hashes, so the exact-value assertion fails and breaks the `cargo test hash collisions` CI job. Gate the test with `#[cfg(not(feature = "force_hash_collisions"))]`, matching every other exact-hash-value test in this module, so it is excluded from the forced-collision build. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
The `use sqlparser::ast::Ident;` in the `utils` test module is only used by `test_quote_identifier`, which is `#[cfg(feature = "sql")]`. Building the `datafusion-common` tests without the `sql` feature therefore left the import unused and emitted an `unused_imports` warning. Gate the import with `#[cfg(feature = "sql")]` so it is compiled exactly when its sole user is, removing the warning and keeping the import and its user consistent across feature combinations. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
neilconway
approved these changes
Jun 19, 2026
neilconway
left a comment
neilconway
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the fix @Phoenix500526 ! lgtm
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jun 20, 2026
…_collisions) (#23053) ## Which issue does this PR close? - N/A — a CI test-gating fix found while building the workspace under `force_hash_collisions`. Follow-up to #23044, which fixed the equivalent `datafusion-common` test. ## Rationale for this change Three `approx_distinct` tests assert exact distinct-count results that HyperLogLog can only produce with real hashing. Under the `force_hash_collisions` feature, `create_hashes` forces every hash equal, so the cardinality estimate collapses to 1 and the assertions fail — breaking the `cargo test hash collisions` and `extended_tests` CI jobs: ``` update_batch_nullable_filter_excludes_null_filter_rows utf8view_groups_short_string_hashed_consistently_across_batches utf8view_acc_split_batches_match_single_mixed_batch ``` `datafusion-functions-aggregate` did not declare `force_hash_collisions`, so a plain `#[cfg(not(feature = "force_hash_collisions"))]` would also raise an `unexpected_cfgs` lint and never actually fire. ## What changes are included in this PR? - Forward the feature in `datafusion-functions-aggregate/Cargo.toml`: `force_hash_collisions = ["datafusion-common/force_hash_collisions"]`, so the crate recognises it (and it gets enabled under the workspace `force_hash_collisions` build). - Gate the three tests (and their test-only imports / helpers) with `#[cfg(not(feature = "force_hash_collisions"))]`, matching the pattern already used for the equivalent `datafusion-common` tests. Test-only changes; no production code is touched. ## Are these changes tested? - `cargo test -p datafusion-functions-aggregate --features force_hash_collisions approx_distinct` — the three tests are now excluded; the rest pass. - `cargo test -p datafusion-functions-aggregate approx_distinct` (no feature) — all tests still present and pass. - `cargo clippy -p datafusion-functions-aggregate --all-targets --features force_hash_collisions -- -D warnings` — clean. ## Are there any user-facing changes? No. --------- Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
datafusion-commonfound while building the crate's tests under non-default feature sets.Rationale for this change
Two
#[cfg]feature-gating issues indatafusion-commontest code:test_create_hashes_with_quality_hash_stateasserts thatcreate_hashesreproduces a specific high-quality hash distribution. Under the
force_hash_collisionsfeature,create_hashesis replaced by a stub thatwrites all-zero hashes, so the exact-value assertion fails — this breaks the
cargo test hash collisionsCI job. Every other exact-hash-value test inthat module is already gated with
#[cfg(not(feature = "force_hash_collisions"))];this one was missing the gate.
The test-only
use sqlparser::ast::Ident;inutils::testsis used solely bytest_quote_identifier, which is#[cfg(feature = "sql")]. Building the testswithout the
sqlfeature leaves the import unused and emits anunused_importswarning.What changes are included in this PR?
test_create_hashes_with_quality_hash_statewith#[cfg(not(feature = "force_hash_collisions"))].Identimport with#[cfg(feature = "sql")]to match itssole user.
Both are test-only changes; no production code is touched.
Are these changes tested?
Verified via the existing tests:
--features force_hash_collisions,test_create_hashes_with_quality_hash_stateis now excluded (no longer fails the forced-collision build); without the
feature it still compiles and passes.
datafusion-commontests without thesqlfeature no longer emitsthe
unused_importswarning; withsqlthe import andtest_quote_identifierare both present.
Are there any user-facing changes?
No.