fix: ignore constant aggregate completion boundaries - #25019
Conversation
|
Since #24697 is making the broader fix for this problem, we can possibly have it fixed in that PR instead of here |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25019 +/- ##
==========================================
- Coverage 81.90% 81.90% -0.01%
==========================================
Files 1134 1134
Lines 425261 425341 +80
Branches 425261 425341 +80
==========================================
+ Hits 348325 348363 +38
- Misses 56295 56320 +25
- Partials 20641 20658 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sunchao
left a comment
There was a problem hiding this comment.
Thanks @yashrb24, LGTM.
Ignoring constant grouping keys for completion detection correctly rejects unbounded plans that cannot finish groups, while preserving incremental output when a varying grouping key is genuinely ordered.
Reviewed commit 037c8cf with five independent passes. Focused base/head validation passed 2,376 SQL cases per revision, eight paired streaming checks, and 40 low-memory cases per revision, including 16 spilling cases each. No actionable regressions found.
Regular and extended CI suites passed. The forced-hash-collision job timed out in unchanged COUNT DISTINCT capacity tests outside the modified aggregation code.
037c8cf to
41eda67
Compare
Resolving the rebase conflict in `aggregates/mod.rs` took this branch's copy of the whole file, which reverted the changes made upstream while the branch was in flight: - `constant_grouping_expr_is_not_a_completion_boundary` and the imports it needs, added with the constant-boundary fix (apache#25019). That fix's non-test half moved into the builder with the rest of the construction logic, so only the test was missing. - the `test_grouped_aggregation_respects_memory_limit` assertion, which upstream relaxed to match on `e.find_root()`. Aggregate spilling reports a `ResourcesExhausted` wrapped in a `Context`, which the older assertion could not see through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwTc51ca2XHDCyVB7MbJoz
Which issue does this PR close?
Rationale for this change
A grouping column with one fixed value never changes, so it cannot tell aggregation that an earlier group is complete.
Currently the logic uses such columns when choosing how aggregation processes its input. This can select the ordered path even when the remaining grouping columns are not ordered.
What changes are included in this PR?
What is the testing strategy for this PR?
Added tests for a grouping column fixed by a filter. The tests fail without the fix.
Are there any user-facing changes?
No query results or public APIs change. DataFusion now chooses the appropriate aggregation path for fixed grouping columns.