Repository navigation
Conversation
Partial and PartialReduce aggregates declared Unique(group keys), but partial state is not unique on the group keys: partition merges keep the constraint, and a partial stage can emit a group more than once. The stale constraint let an ordering on the group keys satisfy a longer ordering, so a required sort could be removed (e.g. replaced by an order-preserving repartition with prefer_existing_sort). Only declare the constraint for modes that produce final values. Co-authored-by: Isaac <no-reply@databricks.com>
github-actions
Bot
added
core
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
AggregateExecdeclaresUnique(group keys)for every aggregation mode, includingPartialandPartialReduce. Partial state is not unique on the group keys. Operators that merge partitions keep the constraint, and a partial stage can also emit the same group more than once within a partition.ordering_satisfytreats[k]plusUnique(k)as satisfying[k, x], so a required sort can be dropped. For example, withprefer_existing_sort, a sort on[k, sum]above a hash repartition of a Partial aggregate is replaced by an order-preserving repartition, and rows with equalkcome out unsorted.What changes are included in this PR?
AggregateExec::compute_propertiesadds theUniqueconstraint only when the mode's output is final (Final,FinalPartitioned,Single,SinglePartitioned). Constraints projected from the input are unchanged.Default SQL plans always put the Final aggregate above the Partial one. The Final aggregate keeps declaring
Unique, and its input ordering requirement covers only the group keys, so this doesn't change any plan in the sqllogictests.What is the testing strategy for this PR?
partial_aggregate_does_not_declare_unique_group_keysinphysical-plan/src/aggregates/mod.rs: a Partial aggregate over two sorted partitions declares no constraint,SortExec([k, sum])overSortPreservingMergeExec([k])sorts the merged rows, and the Final aggregate still declaresUnique.test_keep_sort_above_repartitioned_partial_aggregateincore/tests/physical_optimizer/enforce_sorting.rs: withprefer_existing_sort,EnsureRequirementskeeps theSortExecabove the hash repartition of the Partial aggregate, and every output partition is sorted.Both tests fail without the change.
Are there any user-facing changes?
No. Partial aggregate output no longer advertises a
Uniqueconstraint on its group keys.