bvolpato opened a new pull request, #26445: URL: https://github.com/apache/datafusion/pull/26445
## Which issue does this PR close? Closes #26444. ## Rationale for this change TopK aggregation can emit separate groups for repeated identical NaN keys. Evicting one of those groups can also panic. Valid `GROUP BY ... ORDER BY ... LIMIT` queries therefore return incorrect results or fail when TopK aggregation is enabled. ## What changes are included in this PR? Canonicalize primitive keys before hashing and compare them with Arrow equality, matching ordinary primitive grouping. Remove occupied entries by their known store index so eviction does not depend on reflexive key equality. Distinct NaN payloads and signs retain their existing grouping semantics; this does not implement the broader canonicalization proposed in #26091. #16256 is the related fix for ordinary grouping. Add SQL regressions for duplicate NaN groups and eviction, plus focused hash-table coverage for Float16/32/64, distinct NaN payloads, signed zero, and NULL aggregate transitions. Two separate prerequisite commits address validation failures reproduced on unchanged main: - Scope the existing borrowed `Expr` key sets' Clippy allowances, following #26288. Arrow reservation bookkeeping does not affect expression hashing or equality. - Reuse the current-process RSS sampling fix from the closed, unmerged #26208, credited in its commit. This prevents a short baseline query from recording zero RSS. Memory limits and assertion thresholds are unchanged. #26354 documents the related sampling problem and separate SMJ variability. ## What is the testing strategy for this PR? Both SQL regressions fail before the fix: one returns an extra `NaN, 1` group, and the other panics during eviction. Both pass with the fix. Validation passed: - `cargo fmt --all` - `cargo clippy --all-targets --all-features -- -D warnings` - The required extended workspace tests with the CI profile and `avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`, including the full SQL logic suite. - `cargo test --profile ci -p datafusion-cli` - `uv run ./dev/rust_lint.sh`, including generated documentation, dependency checks, Rustdoc, and the HTML documentation build. The existing `topk_aggregate` primitive-key benchmark showed no regression in the measured case against main at `5f2158269`. These were shared-host measurements and do not establish a broader performance improvement. ## Are there any user-facing changes? Repeated identical NaN grouping keys form one TopK group, and evicting a NaN group succeeds. Signed-zero keys are normalized consistently with ordinary primitive grouping. There are no public API changes. Draft for manual review before requesting upstream review. AI-assisted contribution. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
