fix: keep a DISTINCT aggregate's arity in SingleDistinctToGroupBy - #25461
Open
namanjain24-sudo wants to merge 1 commit into
Open
namanjain24-sudo wants to merge 1 commit into
namanjain24-sudo wants to merge 1 commit into
Conversation
`is_single_distinct_agg` requires that every distinct argument in the aggregation is the same expression, which a call passing that expression more than once satisfies, since its arguments collapse to one entry in the set the check uses. The rewrite then assumed one argument per distinct call, so `corr(DISTINCT x, x)` failed with an internal error. Repeat the alias the inner group by produces, keeping the call's arity. That is sound because of the same check: the distinct argument tuples such a call aggregates are exactly the distinct values of the expression, which is what grouping by it produces. The assertion now states the invariant the rewrite relies on, and covers an empty argument list rather than panicking in `swap_remove`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25461 +/- ##
=======================================
Coverage 82.35% 82.35%
=======================================
Files 1137 1137
Lines 432716 432754 +38
Branches 432716 432754 +38
=======================================
+ Hits 356342 356374 +32
+ Misses 54839 54838 -1
- Partials 21535 21542 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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?
corr(DISTINCT x, x)fails with internal error inSingleDistinctToGroupBy#25417.Rationale for this change
is_single_distinct_aggrequires that every distinct argument in the aggregation is the same expression. It checks that by collecting the arguments into a set and requiring the set to hold exactly one element:A call that passes that expression more than once satisfies the check, because its arguments collapse to a single entry.
corr(DISTINCT x, x)is such a call, and the rewrite below then assumed one argument per distinct aggregate and returned an internal error.This is also why the neighbouring cases in the issue are unaffected:
corr(DISTINCT x, y)andcorr(DISTINCT x, x + 0.0)put two entries in the set, so the rule bails out early and leaves the plan alone.What changes are included in this PR?
The distinct branch keeps the call's arity, repeating the alias that the inner group by produces, so
corr(DISTINCT x, x)becomes a group byxwithcorr(alias1, alias1)above it.That is sound because of the check above. Every distinct argument is the same expression, so the distinct argument tuples the call aggregates are exactly the distinct values of that expression, which is what grouping by it produces.
The assertion stays, now stating the invariant the rewrite actually relies on rather than an arity the rule does not enforce. It also covers an empty argument list, which would otherwise panic in
args.swap_remove(0)instead of reporting an internal error.What is the testing strategy for this PR?
Rule tests in
single_distinct_to_groupby.rs:single_distinct_repeated_arg_and_groupby:corr(DISTINCT b, b)is rewritten, and the plan keeps both arguments and the original output column name.single_distinct_two_args_and_groupby:corr(DISTINCT b, c)is still left alone.Two queries in
aggregate.sltcover it end to end. One iscovar_samp, chosen because its value depends on the de-duplication rather than only on the absence of the error: over the same input it returns0.5withDISTINCTand0.333333333333without, so the expected value pins the semantics.Measured, rather than assumed:
aggregate.sltqueries andsingle_distinct_repeated_arg_and_groupbyfail with anAssertion failed: args.len() == 1internal error, which is the failure in the issuecargo test -p datafusion-optimizer(915 tests), the sqllogictest suite (521 files), and./ci/scripts/rust_clippy.shpass.Are there any user-facing changes?
Queries that failed with an internal error now run. No plan that the rule rewrote before is rewritten differently: a single argument call produces the same outer aggregate as it did, and the only calls that reach this branch with more than one argument are the ones that previously returned the error.