Skip to content

fix: keep a DISTINCT aggregate's arity in SingleDistinctToGroupBy - #25461

Open
namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-distinct-multi-arg
Open

namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-distinct-multi-arg

Conversation

@namanjain24-sudo

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

is_single_distinct_agg requires 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:

if *distinct {
    for e in args {
        fields_set.insert(e);
    }

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) and corr(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 by x with corr(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.slt cover it end to end. One is covar_samp, chosen because its value depends on the de-duplication rather than only on the absence of the error: over the same input it returns 0.5 with DISTINCT and 0.333333333333 without, so the expected value pins the semantics.

Measured, rather than assumed:

check result
both halves of the fix reverted the two aggregate.slt queries and single_distinct_repeated_arg_and_groupby fail with an Assertion failed: args.len() == 1 internal error, which is the failure in the issue
the rewrite against a reference the rule does not touch the same aggregates over a manually de-duplicated input return the same values

cargo test -p datafusion-optimizer (915 tests), the sqllogictest suite (521 files), and ./ci/scripts/rust_clippy.sh pass.

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.

`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`.
@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Sep 18, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.36585% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.35%. Comparing base (b9cea81) to head (5cc200a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...fusion/optimizer/src/single_distinct_to_groupby.rs 85.36% 0 Missing and 6 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

corr(DISTINCT x, x) fails with internal error in SingleDistinctToGroupBy

2 participants