fix: eliminate group by constant empty input by HairstonE · Pull Request #22132 · apache/datafusion

HairstonE · 2026-05-12T16:34:14Z

Which issue does this PR close?

Closes Incorrect result returned by a group by query (SQLancer-TLP) #11748

Rationale for this change

EliminateGroupByConstant removes GROUP BY expressions that are constants. If all of the GROUP BY expressions are constants, eliminating all of them results in converting a grouped aggregate into an ungrouped (global) aggregate. This changes the semantics of the query: a grouped aggregate query on an empty input returns zero rows, whereas an ungrouped aggregate query produces a single row.

What changes are included in this PR?

EliminateGroupByConstant now declines to eliminate constant GROUP BY expressions, if doing so would result in removing all of the grouping expressions.

Are these changes tested?

Yes. Sqllogictest reproducer for the original issue, updated unit test and optimizer SLT expectations.

Are there any user-facing changes?

Queries with all-constant GROUP BY may now return fewer (correct) rows.

kumarUjjawal

Thanks @HairstonE Looks good.

@2010YOUY01 do you have any thoughts on this approach

alamb · 2026-05-26T19:09:08Z

This is on main:

> create table foo as values (1);
0 row(s) fetched.
Elapsed 0.010 seconds.

> SELECT max(column1) FROM foo GROUP BY false HAVING false;
+------------------+
| max(foo.column1) |
+------------------+
| NULL             |
+------------------+
1 row(s) fetched.
Elapsed 0.004 seconds.

Postgres rejects GROUP BY false:

postgres=# CREATE TABLE foo(column1 int);
insert into foo values (1);
SELECT max(column1) FROM foo GROUP BY false HAVING false;
CREATE TABLE
INSERT 0 1
ERROR:  non-integer constant in GROUP BY
LINE 1: SELECT max(column1) FROM foo GROUP BY false HAVING false;

DuckDB returns 0 rows:

memory D CREATE TABLE foo(column1 int);
memory D insert into foo values (1);
memory D SELECT max(column1) FROM foo GROUP BY false HAVING false;
┌──────────────┐
│ max(column1) │
│    int32     │
└──────────────┘
     0 rows

alamb · 2026-05-26T19:11:51Z

-02)--Aggregate: groupBy=[[]], aggr=[[count(Int64(1)), avg(t.c12)]]
-03)----SubqueryAlias: t
-04)------TableScan: test_table projection=[c12]
+01)Aggregate: groupBy=[[Int64(123), Int64(456), Int64(789)]], aggr=[[count(Int64(1)), avg(t.c12)]]


isn't this a significantly worse plan? it now must compute the grouping columns for each input row (even though they are constants)

I am shooting for the DuckDB behavior. This gets the correct output on empty input. But yeah it will be slower on all inputs. Should I workshop this a bit more?

Yes, I think we should avoid slowing down queries to implement a corner case. What about putting a Filter in the plan? Or maybe switching to use EmptyExec in some cases 🤔

Alright, I'll draft this PR for now and look into those recommendations.
Thank you for the feedback!

Thank you 🙏

Update here -- this is still a significantly worse plan, however, as @neilconway points out in #23871 (comment) such a plan is likely not common (it produces a single group) so this is probably ok to change

alamb

Sorry I forgot to submit my review

I am worried about this PR causing performance regressions for more common queries. Can we please try and find a way to scope the fix to just the queries where this is a problem?

alamb · 2026-07-01T17:56:02Z

+                    };
+                    let sentinel_name =
+                        config.alias_generator().next("__row_count_sentinel");
+                    let count_udaf = registry.udaf("count")?;


this basically assumes that any function with the name count() will behave as the built in count 🤔 that doesn't seem safe to me (unless it is done elsewhere)

replace_distinct_aggregate.rs line 134 does something similar with let first_value_udaf: Arc<datafusion_expr::AggregateUDF> = config.function_registry().unwrap().udaf("first_value")?;
Not saying that it's the right way to go just that it exists elsewhere. Otherwise, we could import count_udaf() from datafusion-functions-aggregate.

alamb · 2026-07-01T17:56:11Z

+                    let sentinel = count_udaf
+                        .call(vec![lit(1_i64)])
+                        .alias(sentinel_name.as_str());
+                    let mut aggr_with_sentinel = aggregate.aggr_expr.clone();


Can we avoid the clone

alamb · 2026-07-01T17:57:44Z

@@ -60,26 +60,42 @@ FROM test_table t
 group by 1, 2, 3
 ----
 logical_plan


these plans are changing but I don't see the actual query results changing. Is that expected?

These all have the new filter, but their outputs are unchanged because the filter passes whenever count > 0. The empty input tests are in aggregate.slt. Those tests on main would return 1 row of NULL, these changes return 0 rows.

HairstonE · 2026-07-14T23:17:09Z

This will only impact queries where every GROUP BY expression is constant. Any of those queries that don't already compute count(1)/count(*) will be slower ~0.9ns/row (this is from a microbench with 50M rows) but we get correctness on the corner case.

alamb · 2026-07-15T13:38:33Z

This will only impact queries where every GROUP BY expression is constant. Any of those queries that don't already compute count(1)/count(*) will be slower ~0.9ns/row (this is from a microbench with 50M rows) but we get correctness on the corner case.

Is there some way to fix this issue rather than adding a new filter (which has a per row cost)? For example, can we add a flag to the aggregate stream?

HairstonE · 2026-07-15T16:00:58Z

If we want to add a flag to the aggregate stream I think that moves this to the physical planner, I can give this a try. It seems like something similar was attempted here #11897.
It also looks like DF's count could be optimized to reduce the per row cost(this would be a separate issue+PR and I haven't looked into too deeply yet).

HairstonE · 2026-07-17T13:59:22Z

I experimented with the flag in the physical planner and it is the best option. I'll close this and open another PR with those changes.

codecov-commenter · 2026-07-24T21:16:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.67%. Comparing base (88365dd) to head (f4b957d).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #22132      +/-   ##
==========================================
+ Coverage   80.65%   80.67%   +0.02%     
==========================================
  Files        1093     1095       +2     
  Lines      371619   372458     +839     
  Branches   371619   372458     +839     
==========================================
+ Hits       299730   300485     +755     
- Misses      53993    54043      +50     
- Partials    17896    17930      +34

☔ 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.

alamb

Thank you @HairstonE

FYI @neilconway

alamb · 2026-07-26T16:02:15Z

-02)--Aggregate: groupBy=[[]], aggr=[[count(Int64(1)), avg(t.c12)]]
-03)----SubqueryAlias: t
-04)------TableScan: test_table projection=[c12]
+01)Aggregate: groupBy=[[Int64(123), Int64(456), Int64(789)]], aggr=[[count(Int64(1)), avg(t.c12)]]


Update here -- this is still a significantly worse plan, however, as @neilconway points out in #23871 (comment) such a plan is likely not common (it produces a single group) so this is probably ok to change

neilconway

I took the liberty of updating the PR description to match the current approach. Two minor nits; otherwise lgtm!

neilconway · 2026-07-27T15:59:00Z

+                // Only eliminate when at least one required group expression remains. A grouping
+                // aggregate emits 0 rows on empty input; a global aggregate emits 1 NULL row.



Slightly inaccurate: ungrouped COUNT returns a row with a 0, not a NULL. How about this:

Suggested change

// Only eliminate when at least one required group expression remains. A grouping

// aggregate emits 0 rows on empty input; a global aggregate emits 1 NULL row.

// Return now if no simplification can be done. We also bail out

// if applying the optimization would eliminate all of the

// grouping expressions (e.g., GROUP BY on only constant

// expressions): this would turn a grouped aggregate into an

// ungrouped aggregate, which changes query semantics (grouped

// aggregates produce an empty result set on an empty input,

// whereas ungrouped aggregates return a single row).

neilconway · 2026-07-27T16:00:54Z

Rename this test func so it doesn't imply that we eliminate constant grouping keys in this case?

HairstonE · 2026-07-27T17:22:55Z

@neilconway I made both those changes you suggested. Should be ready to merge!

neilconway · 2026-07-28T20:22:22Z

Thanks @HairstonE ! Sorry again for the back-and-forth on this issue, I think we ended up with the right approach.

HairstonE · 2026-07-28T20:41:18Z

@neilconway No worries at all, the back and forth was great to learn more about the codebase. Thank you and @alamb for the reviews and feedback!

alamb · 2026-07-29T07:43:57Z

I agree -- thanks again @HairstonE and @neilconway

github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels May 12, 2026

kumarUjjawal changed the title ~~Fix/11748 eliminate group by constant empty input~~ fix: eliminate group by constant empty input May 13, 2026

kumarUjjawal reviewed May 13, 2026

View reviewed changes

Comment thread datafusion/sqllogictest/test_files/aggregate.slt

HairstonE requested a review from kumarUjjawal May 19, 2026 15:17

kumarUjjawal approved these changes May 19, 2026

View reviewed changes

kumarUjjawal requested a review from 2010YOUY01 May 19, 2026 15:31

alamb reviewed May 26, 2026

View reviewed changes

HairstonE marked this pull request as draft May 27, 2026 18:15

HairstonE marked this pull request as ready for review June 16, 2026 20:15

HairstonE force-pushed the fix/11748-eliminate-group-by-constant-empty-input branch from 2dd9ecb to a67eb74 Compare June 29, 2026 16:09

HairstonE requested a review from alamb June 29, 2026 17:22

alamb reviewed Jul 13, 2026

View reviewed changes

HairstonE closed this Jul 17, 2026

HairstonE mentioned this pull request Jul 24, 2026

Fix/11748 agg emit flag #23871

Closed

HairstonE reopened this Jul 24, 2026

HairstonE added 3 commits July 26, 2026 09:57

remove unneeded guard condition and update unit test

4687667

add sqllogictest for reproducer, test outputs

2820196

add non HAVING and non-empty GROUP BY false tests per review.

5644a58

HairstonE force-pushed the fix/11748-eliminate-group-by-constant-empty-input branch from a67eb74 to 5644a58 Compare July 26, 2026 15:05

alamb approved these changes Jul 26, 2026

View reviewed changes

neilconway approved these changes Jul 27, 2026

View reviewed changes

update comment and test name

f4b957d

neilconway enabled auto-merge July 27, 2026 17:45

neilconway added this pull request to the merge queue Jul 27, 2026

Merged via the queue into apache:main with commit 9ab2068 Jul 27, 2026
40 checks passed

		// Only eliminate when at least one required group expression remains. A grouping
		// aggregate emits 0 rows on empty input; a global aggregate emits 1 NULL row.

-                // Only eliminate when at least one required group expression remains. A grouping
-                // aggregate emits 0 rows on empty input; a global aggregate emits 1 NULL row.
+                  // Return now if no simplification can be done. We also bail out
+                  // if applying the optimization would eliminate all of the
+                  // grouping expressions (e.g., GROUP BY on only constant
+                  // expressions): this would turn a grouped aggregate into an
+                  // ungrouped aggregate, which changes query semantics (grouped
+                  // aggregates produce an empty result set on an empty input,
+                  // whereas ungrouped aggregates return a single row).

Uh oh!

Conversation

HairstonE commented May 12, 2026 • edited by neilconway Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

kumarUjjawal left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HairstonE commented Jul 14, 2026

Uh oh!

alamb commented Jul 15, 2026

Uh oh!

HairstonE commented Jul 15, 2026

Uh oh!

HairstonE commented Jul 17, 2026

Uh oh!

codecov-commenter commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neilconway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HairstonE commented Jul 27, 2026

Uh oh!

Uh oh!

neilconway commented Jul 28, 2026

Uh oh!

HairstonE commented Jul 28, 2026

Uh oh!

alamb commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

HairstonE commented May 12, 2026 •

edited by neilconway

Loading

alamb commented May 26, 2026 •

edited

Loading

codecov-commenter commented Jul 24, 2026 •

edited

Loading