Skip to content

fix: clear groupings when clearing orderings - #26852

Open
xavlee wants to merge 1 commit into
apache:mainfrom
xavlee:xavlee/clear-groupings-with-orderings
Open

xavlee wants to merge 1 commit into
apache:mainfrom
xavlee:xavlee/clear-groupings-with-orderings

Conversation

@xavlee

@xavlee xavlee commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR relate to?

Part of #24438. Follows up on the invalidation API review in #24698.

Builds on #24698 (merged). This PR and #24497 both build on the grouping metadata introduced there.

Rationale for this change

Execution operators can clone their input's equivalence properties and call clear_orderings() when rearranging rows. Clearing grouping assertions at the same boundary prevents consumers from relying on contiguous groups that the output no longer guarantees.

Note: clear_groupings() here is conservative since grouping does not imply ordering.

What changes are included in this PR?

  • Make EquivalenceProperties::clear_orderings() clear explicit grouping tuples and their normalized cache along with ordering information.
  • Document the invalidation behavior and the narrower clear_groupings() operation, which preserves ordering information.
  • Let partition coalescing and sort-preserving merge use the combined clear.
  • Cover partially clustered aggregation that preserves ordering on one key while clearing an unrelated grouping assertion.

Stack

  1. #24737 — test: cover unsorted contiguous groups in one partition (merged)
  2. #24697 — refactor: use group clustering terminology in aggregate execution (merged)
  3. #24698 — feat: add grouping equivalence properties (merged)
  4. #24497 — feat: stream aggregates over grouped input
  5. #26852 — fix: clear groupings when clearing orderings ← this PR

What is the testing strategy for this PR?

  • clear_orderings_clears_groupings checks that ordering-derived grouping and explicit grouping are invalidated, including the normalized grouping cache.
  • clear_groupings_preserves_orderings checks that explicit grouping can be cleared while ordering and ordering-derived grouping remain available.
  • coalesce_clears_explicit_grouping covers one and multiple input partitions.
  • clustered_aggregate_clears_explicit_grouping covers partial and single aggregation, preserving the ordering on a while clearing the explicit grouping on c.

Removing the new API clear makes its regression fail. Removing the aggregate's explicit grouping clear makes both clustered aggregate cases fail.

Are there any user-facing changes?

Yes. clear_orderings() also clears explicit grouping assertions. Operators using this API discard those assertions conservatively, including single-input CoalescePartitionsExec.

Since grouping information is new, this should not affect any existing operators.

Review this layer

View this follow-up's changes against main.

@xavlee
xavlee marked this pull request as ready for review October 28, 2026 15:06
@xavlee
xavlee force-pushed the xavlee/clear-groupings-with-orderings branch 3 times, most recently from 1f8eb57 to dccf624 Compare November 2, 2026 18:16
@xavlee
xavlee force-pushed the xavlee/clear-groupings-with-orderings branch from dccf624 to 86f93b6 Compare November 3, 2026 15:21
@xavlee

xavlee commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

@jayzhan211 Bumping this followup fix PR 🙇

Tristan1900 added a commit to DataDog/datafusion that referenced this pull request Nov 3, 2026
Backport the Apache DataFusion apache#24438 stack onto DataDog branch-55:
- apache#24737: unsorted contiguous-group regression coverage
- apache#24697: group-clustering terminology and aggregate APIs
- apache#24698: grouping equivalence properties and propagation
- apache#24497: incremental aggregation over explicitly grouped input
- apache#26852: invalidate grouping assertions when clearing orderings

Adapt the changes to DF55's existing aggregate streams. Single-stage
clustered aggregation continues to use GroupedHashAggregateStream;
partial and final aggregation retain the existing specialized streams.
Do not import the unrelated DF56 aggregate-stream migration.

Include the constant-key, grouping-set, and PartialReduce guards. Extend
blocked-input tests across single/final and migrated/legacy execution,
and retain sort-option and metadata-invalidation coverage. Update DF55
plan snapshots, the wrapped memory-limit error assertion, and the API
upgrade guide.

Validation: 503 SQL test files passed, 3347 focused expression/plan unit
tests passed, and all-targets/all-features clippy passed. Extended tests
exposed a dictionary sort-fuzzer failure also reproduced on the unchanged
branch-55 base (root seed 1793724857; dataset 15872832093508421016; query
12465732596605393044; config 11717754459111916353).

Upstream-Commit: 59d74ca
Upstream-Commit: d93699c
Upstream-Commit: 320a7d2
Upstream-Commit: 64a5955
Upstream-Commit: 3c3898e
Upstream-Commit: 86f93b6
Co-authored-by: xavlee <52435309+xavlee@users.noreply.github.com>
Tristan1900 pushed a commit to DataDog/datafusion that referenced this pull request Nov 3, 2026
Backport apache#26852

Invalidate both grouping metadata and its cache when ordering information
is cleared. Cover coalescing and clustered aggregate property handling.

The completed five-commit stack has the same Git tree as the previously
validated e6b218d backport.
Validation there included 3,347 focused tests and 503 SQL test files.
The extended sort-fuzzer failure also reproduces on unchanged branch-55
(root seed 1793724857; dataset 15872832093508421016; query
12465732596605393044; config 11717754459111916353).

Upstream-Commit: 86f93b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant