Fix/11748 agg emit flag#23871
Conversation
… GROUP BY so we can emit no rows when the input is empty
github-actions
Bot
added
optimizer
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23871 +/- ##
=========================================
Coverage 80.71% 80.71%
=========================================
Files 1089 1090 +1
Lines 368750 370635 +1885
Branches 368750 370635 +1885
=========================================
+ Hits 297631 299166 +1535
- Misses 53375 53618 +243
- Partials 17744 17851 +107 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I'm curious -- why do we want to follow the DuckDB behavior here? Integer literal GROUP BY expressions are interpreted as column ordinals, so to me it seems more consistent to reject non-integer literal GROUP BY expressions, as Postgres does. |
|
Hi Neil!
DuckDB has the correct output so I used it for reference. I believe Postgres only blocks bare literals. When it accepts this type of query Postgres has the same behavior as DuckDB. This bug would still persist in constant expression group bys even if we reject the same way Postgres does. Adding this rejection now could also cause query breaking changes. @alamb Do you have any thoughts on this? |
|
I am sorry for all the back and forth @HairstonE Rereading #11748 it uses this query: I am really struggling to figure out why the solutions seem so complicated and I don't understand why emitting an output row is a function of all the groups being constant. Specifically since there are no input rows in this example, it shouldn't matter what the group columns are, no output should be emitted (except for COUNT which is a specia case) It seems to me the core problem is that > create table t1(v1 int);
0 row(s) fetched.
Elapsed 0.021 seconds.
> SELECT AVG(v1) FROM t1 GROUP BY false;
+------------+
| avg(t1.v1) |
+------------+
| NULL |
+------------+
1 row(s) fetched.
Elapsed 0.040 seconds.
|
| logical_plan | ||
| 01)Projection: Date32("2023-05-04") AS dt, Boolean(true) AS today_filter, count(Int64(1)) | ||
| 02)--Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]] | ||
| 01)Projection: to_date(Utf8("2023-05-04")) AS dt, date_part(Utf8("DAY"),now()) < Int64(1000) AS today_filter, count(Int64(1)) |
There was a problem hiding this comment.
this seems like a regression (the expression is no longer folded)
To me, the simplest fix here is just to decline to optimize away the grouping if all of the grouping expressions are constants. This is a corner case anyway; the perf hit is not catastrophic, and I'd be surprised to see a real-world workload where this pattern occurs. And yes, I realize that we did something similar in a prior PR 🙃 |
if you are happy with the PR #22132 then let's reopen it and merge I am sorry for all the runaround @HairstonE |
|
I can reopen that PR. Right now it is not what Neil is suggesting, I would need to revert |
Personally I'd prefer that simple approach, but if anyone (e.g., you or @alamb) see a reason to do something more complicated, would love to discuss. |
My goal is: 1) simple implementation and 2) no performance regressions (for legitmate useful queries) I may have misunderstood the original PR and its impact on performance |
4 participants
Which issue does this PR close?
Rationale for this change
EliminateGroupByConstantstrips constant expressions from a GROUP BY. When it strips all of them, it turns a grouping aggregate into a global aggregate, which returns 1 row on empty input instead of 0.Original attempt: #11897. My previous PR #22132 had the correct output but at a per-row cost, whereas this solution does not have that cost.
What changes are included in this PR?
EliminateGroupByConstantnow leaves an all-constant GROUP BY alone. When every group key is a literal and there's at least one aggregate, the physical planner plans a global AggregateExec carrying a new emit_no_rows_on_empty_input flag, with a projection re-attaching the constant columns. Now a flagged aggregate that saw no input rows emits 0 rows instead of the one default row a global aggregate returns.The flag survives plan rewrites and round-trips through the proto.
A GROUP BY that isn't all-constant takes exactly the path it did before.
Are these changes tested?
Yes. A sqllogictest reproducer for the original issue (
GROUP BY false HAVING falsereturning 0 rows), plus the non-HAVING case (GROUP BY falseon empty input) and the non-empty case. Updated the optimizer SLT expectations and unit tests.Are there any user-facing changes?
Queries with an all-constant GROUP BY over empty input now correctly return 0 rows instead of 1 row of NULL.