From 688f058e9233a5aa8e088c8d6e67753e6a305096 Mon Sep 17 00:00:00 2001 From: DevShiba Date: Mon, 7 Sep 2026 09:25:07 -0300 Subject: [PATCH] fix: skip count-distinct byte tests under force_hash_collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #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 #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 #25011 --- datafusion/functions-aggregate-common/Cargo.toml | 4 ++++ .../src/aggregate/count_distinct/bytes.rs | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/datafusion/functions-aggregate-common/Cargo.toml b/datafusion/functions-aggregate-common/Cargo.toml index 1714e1800a4fe..0898e6b4c5522 100644 --- a/datafusion/functions-aggregate-common/Cargo.toml +++ b/datafusion/functions-aggregate-common/Cargo.toml @@ -40,6 +40,10 @@ workspace = true [lib] name = "datafusion_functions_aggregate_common" +[features] +# Used for testing ONLY: causes all values to hash to the same value (test for collisions) +force_hash_collisions = ["datafusion-common/force_hash_collisions"] + [dependencies] arrow = { workspace = true } datafusion-common = { workspace = true } diff --git a/datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs b/datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs index d955d343ad629..c230a27c02616 100644 --- a/datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs +++ b/datafusion/functions-aggregate-common/src/aggregate/count_distinct/bytes.rs @@ -166,7 +166,16 @@ impl Accumulator for BytesViewDistinctCountAccumulator { } } -#[cfg(test)] +/// With `force_hash_collisions` every distinct value collides into the same +/// hash bucket, so both `ArrowBytesSet`/`ArrowBytesViewSet` degrade to a +/// linear scan per insert. These tests insert up to 500,000 distinct values +/// per accumulator (twice, once lazily constructed and once pre-allocated), +/// which is O(n) under a real hash and O(n^2) under a forced collision, +/// turning a sub-second run into a multi-hour one and hanging CI (see +/// apache/datafusion#25011). Skipped under that feature, matching the +/// `count_distinct_spill` precedent in +/// `datafusion/core/tests/memory_limit/mod.rs`. +#[cfg(all(test, not(feature = "force_hash_collisions")))] mod tests { use super::*; use arrow::array::{StringArray, StringViewArray};