Skip to content

fix: skip count-distinct byte tests under force_hash_collisions - #25020

Merged
kosiew merged 1 commit into
apache:mainfrom
DevShiba:fix/skip-count-distinct-bytes-tests-under-forced-collisions
Sep 7, 2026
Merged

fix: skip count-distinct byte tests under force_hash_collisions#25020
kosiew merged 1 commit into
apache:mainfrom
DevShiba:fix/skip-count-distinct-bytes-tests-under-forced-collisions

Conversation

@DevShiba

@DevShiba DevShiba commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The cargo test hash collisions (amd64) CI job hangs for hours (sometimes hitting the 360-minute job limit and getting cancelled) in two tests in datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs:

aggregate::count_distinct::bytes::tests::ungrouped_utf8_accumulator_is_never_worse_than_a_pre_allocated_set
aggregate::count_distinct::bytes::tests::ungrouped_utf8_view_accumulator_is_never_worse_than_a_pre_allocated_set

Root cause: both tests insert up to 500,000 distinct values into ArrowBytesSet/ArrowBytesViewSet, twice per cardinality in CARDINALITIES (once into a lazily-constructed set, once into a pre-allocated one), to compare their reported .size(). Under normal hashing this is O(n) per insert. Under force_hash_collisions (datafusion/common/src/hash_utils.rs#L1184-L1195) every value hashes to the same bucket, so the underlying hash table degrades to a linear scan per insert - O(n^2) overall for a set built up to n elements.

I confirmed this is quadratic, not just slow, with a throwaway local timing probe over the same insert pattern under --features datafusion-common/force_hash_collisions (removed before this PR, shown here for reference):

n time
100 297us
500 3.7ms
1,000 14ms
2,000 54ms
5,000 347ms

Each 2x step in n is roughly a 4x step in time, consistent with O(n^2), and consistent with the multi-hour runtimes reported in #25011 for n up to 500,000.

On the question raised in #25011 ("what behavior or regression boundaries are the 100,000 and 500,000 cardinalities intended to protect, and what approach would preserve that coverage?"): the assertions in assert_lazy_is_not_worse compare allocator sizes reported by a real hash-table implementation against a pre-allocated one, at cardinalities chosen to span both sides of the warm-up capacity (PER_GROUP_SCALE) and the point where the two constructors converge (UNGROUPED_SCALE). None of that is about hash collision behavior - forcing every key into one bucket doesn't exercise a code path these tests are meant to protect, it just makes every insert scan the one bucket's full contents, which is why the cost goes quadratic without adding coverage.

This is the same situation the force_hash_collisions feature already has an established answer for: count_distinct_spill in datafusion/core/tests/memory_limit/mod.rs (added in #24918) is gated with #[cfg(not(feature = "force_hash_collisions"))] because its assertions depend on a real hash distribution across partitions and don't mean anything under forced collisions. This PR applies the identical pattern here, rather than reducing cardinality or otherwise changing what real-hashing runs cover.

What changes are included in this PR?

  • datafusion/functions-aggregate-common/Cargo.toml: add a [features] section declaring force_hash_collisions = ["datafusion-common/force_hash_collisions"], forwarding to datafusion-common's feature of the same name. This crate previously declared no features of its own. Cargo does not propagate a dependency's active feature into a consuming crate's own cfg(feature = ...) checks, so without this forwarding declaration, a #[cfg(feature = "force_hash_collisions")] inside this crate would never see the workspace-level --features force_hash_collisions flag the affected CI job passes (cargo test --workspace --features=force_hash_collisions,avro). This mirrors the exact forwarding pattern already used in datafusion/core/Cargo.toml for the same feature name.
  • datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs: gate the whole mod tests block (it contains only these two tests and their shared helpers - nothing else needs to stay compiled either way) with #[cfg(all(test, not(feature = "force_hash_collisions")))], with a doc comment explaining the O(n^2) mechanism and linking back to this issue.

No production code changes. No reduction in cardinality or coverage for the normal (non-collision-forced) test run - both tests still run exactly as before, across all 7 cardinalities up to 500,000, whenever force_hash_collisions is off.

What is the testing strategy for this PR?

This is a test-only change, verified by running the tests both ways:

  • Without the feature: cargo test -p datafusion-functions-aggregate-common --lib -- count_distinct::bytes still runs and passes both tests in ~0.8s.
  • With the feature: cargo test -p datafusion-functions-aggregate-common --lib --features force_hash_collisions -- count_distinct::bytes runs 0 tests with a clean compile - confirming the gate compiles out cleanly rather than silently failing to match.
  • Against the exact affected CI job command (cd datafusion && cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --exclude datafusion-cli --workspace --lib --tests --features=force_hash_collisions,avro): the datafusion-functions-aggregate-common test binary reports 47 tests (49 minus the 2 gated ones) all passing, with neither ungrouped_utf8_accumulator_is_never_worse_than_a_pre_allocated_set nor its Utf8View counterpart appearing anywhere in the run - confirming the workspace-level feature flag correctly reaches the new local feature via Cargo's feature unification, not just the crate-local invocation.
  • cargo fmt --check and the exact CI clippy invocation (ci/scripts/rust_clippy.sh, i.e. cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings) both pass clean across the whole workspace.

Are there any user-facing changes?

None. This only changes which tests compile under a testing-only feature flag; there is no change to any public API or runtime behavior.

`ungrouped_utf8_accumulator_is_never_worse_than_a_pre_allocated_set`
and its `Utf8View` counterpart in
`datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs`
insert up to 500,000 distinct values into `ArrowBytesSet`/`ArrowBytesViewSet`
twice per cardinality (once lazily built, once pre-allocated). Under normal
hashing this is O(n) per insert. Under `force_hash_collisions` every value
hashes to the same bucket, degrading the set to a linear scan per insert,
i.e. O(n^2) overall.

Empirically measured locally with a throwaway timing probe (since removed):

    n=100    ->  297us
    n=500    ->  3.7ms
    n=1000   ->  14ms
    n=2000   ->  54ms
    n=5000   -> 347ms

which is consistent with the quadratic growth reported in apache#25011 (some CI
runs completing in ~2h39m/4h31m for the two tests, others exceeding the
360-minute job limit and getting cancelled).

This mirrors the existing `force_hash_collisions` precedent for exactly
this class of problem: `count_distinct_spill` in
`datafusion/core/tests/memory_limit/mod.rs` (added in apache#24918) is gated with
`#[cfg(not(feature = "force_hash_collisions"))]` because its assertions
depend on a real hash distribution across partitions. The same reasoning
applies here — these tests assert on allocator sizes that only make sense
under real hashing, and forcing every key into one bucket does not exercise
any behavior the test is meant to protect, it only inflates the runtime.

Changes:
- `datafusion/functions-aggregate-common/Cargo.toml`: declare a local
  `force_hash_collisions` feature forwarding to
  `datafusion-common/force_hash_collisions`, matching the same forwarding
  pattern used in `datafusion/core/Cargo.toml`. Needed because Cargo does
  not propagate a dependency's active feature into a consuming crate's own
  `cfg(feature = ...)` checks - the crate must declare (and forward) the
  feature itself for its own `#[cfg(feature = "force_hash_collisions")]` to
  respond to the workspace-level `--features force_hash_collisions` flag
  the affected CI job passes.
- `bytes.rs`: gate the whole `mod tests` block with
  `#[cfg(all(test, not(feature = "force_hash_collisions")))]`, since it
  contains only these two tests and their shared helpers.

No production code changes; no reduction in cardinality or coverage under
normal (non-collision-forced) test runs, where both tests still run exactly
as before across all 7 cardinalities up to 500,000.

Verified:
- `cargo test -p datafusion-functions-aggregate-common --lib -- count_distinct::bytes`
  (feature off): both tests still run and pass, 0.75s.
- `cargo test -p datafusion-functions-aggregate-common --lib --features force_hash_collisions -- count_distinct::bytes`
  (crate-local feature on): 0 tests run, clean compile.
- The exact affected CI job command, `cargo test --profile ci --exclude
  datafusion-examples --exclude datafusion-benchmarks --exclude
  datafusion-sqllogictest --exclude datafusion-cli --workspace --lib --tests
  --features=force_hash_collisions,avro`: the crate's test binary reports
  47 tests (49 minus the 2 gated ones), all passing, with neither
  `ungrouped_utf8_accumulator_is_never_worse_than_a_pre_allocated_set` nor
  its view counterpart appearing in the run.
- `cargo fmt --check` and the exact `ci/scripts/rust_clippy.sh`
  (`cargo clippy --all-targets --workspace --features
  avro,integration-tests,extended_tests -- -D warnings`): both clean.

Closes apache#25011
@github-actions github-actions Bot added the functions Changes to functions implementation label Sep 7, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.71%. Comparing base (13ea24a) to head (688f058).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25020      +/-   ##
==========================================
- Coverage   81.71%   81.71%   -0.01%     
==========================================
  Files        1127     1127              
  Lines      416051   416051              
  Branches   416051   416051              
==========================================
- Hits       339974   339962      -12     
- Misses      56091    56097       +6     
- Partials    19986    19992       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adriangb
adriangb added this pull request to the merge queue Sep 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 7, 2026
@kosiew
kosiew added this pull request to the merge queue Sep 7, 2026
Merged via the queue into apache:main with commit 6ab4ce6 Sep 7, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: count-distinct byte tests run for hours and hit the 360-minute limit

4 participants