You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "HairstonE (via GitHub)" <gi...@apache.org> on 2026/05/12 16:34:17 UTC
[PR] Fix/11748 eliminate group by constant empty input [datafusion]
HairstonE opened a new pull request, #22132:
URL: https://github.com/apache/datafusion/pull/22132
## Which issue does this PR close?
Closes #11748
## Rationale for this change
`EliminateGroupByConstant` strips constant expressions from GROUP BY. When it strips all of them, it converts a grouping aggregate into a global aggregate. A grouping aggregate returns 0 rows on empty input, a global aggregate returns 1. `GROUP BY false HAVING false` should return 0 rows. It was returning 1 row of NULL.
Found the root cause by reading PR #11897.
## What changes are included in this PR?
The guard now bails out whenever all GROUP BY expressions are constants, regardless of whether aggregate functions exist. One line change, plus updated tests.
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3312921355
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,10 +60,9 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
-01)Projection: Int64(123), Int64(456), Int64(789), count(Int64(1)), avg(t.c12)
-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)]]
Review Comment:
Thank you 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE closed pull request #22132: fix: eliminate group by constant empty input
URL: https://github.com/apache/datafusion/pull/22132
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #22132:
URL: https://github.com/apache/datafusion/pull/22132
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3583218565
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
@@ -64,23 +66,52 @@ impl OptimizerRule for EliminateGroupByConstant {
.group_expr
.iter()
.partition(|expr| is_redundant_group_expr(expr, &group_by_columns));
-
if redundant.is_empty()
|| (required.is_empty() && aggregate.aggr_expr.is_empty())
{
return Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
}
- let simplified_aggregate = LogicalPlan::Aggregate(Aggregate::try_new(
- aggregate.input,
- required.into_iter().cloned().collect(),
- aggregate.aggr_expr.clone(),
- )?);
-
- let projection_expr =
- aggregate.group_expr.into_iter().chain(aggregate.aggr_expr);
+ let simplified_plan = if required.is_empty() {
+ let Some(registry) = config.function_registry() else {
+ return Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
+ };
+ let sentinel_name =
+ config.alias_generator().next("__row_count_sentinel");
+ let count_udaf = registry.udaf("count")?;
Review Comment:
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-5074478368
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/22132?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `86.48649%` with `5 lines` in your changes missing coverage. Please review.
:warning: Please [upload](https://docs.codecov.com/docs/codecov-uploader) report for BASE (`main@93e26f4`). [Learn more](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#section-missing-base-commit) about missing BASE report.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/22132?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...usion/optimizer/src/eliminate\_group\_by\_constant.rs](https://app.codecov.io/gh/apache/datafusion/pull/22132?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Feliminate_group_by_constant.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL2VsaW1pbmF0ZV9ncm91cF9ieV9jb25zdGFudC5ycw==) | 86.48% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22132?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #22132 +/- ##
=======================================
Coverage ? 80.71%
=======================================
Files ? 1090
Lines ? 370395
Branches ? 370395
=======================================
Hits ? 298961
Misses ? 53606
Partials ? 17828
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/22132?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3312872700
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,10 +60,9 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
-01)Projection: Int64(123), Int64(456), Int64(789), count(Int64(1)), avg(t.c12)
-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)]]
Review Comment:
Alright, I'll draft this PR for now and look into those recommendations.
Thank you for the feedback!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-5109446656
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-5109271237
Thanks @HairstonE ! Sorry again for the back-and-forth on this issue, I think we ended up with the right approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3232005125
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
Review Comment:
Also could you add a non-empty positive case alongside the empty-table reproducer?
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
Review Comment:
We should also add this test `SELECT AVG(v1) FROM t1 GROUP BY false;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-5004041047
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3583337823
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,26 +60,42 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3235124272
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
Review Comment:
~~Yeah I'll add those now.~~ Added those tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3306792443
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,10 +60,9 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
-01)Projection: Int64(123), Int64(456), Int64(789), count(Int64(1)), avg(t.c12)
-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)]]
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3306250355
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,10 +60,9 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
-01)Projection: Int64(123), Int64(456), Int64(789), count(Int64(1)), avg(t.c12)
-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)]]
Review Comment:
isn't this a significantly worse plan? it now must compute the grouping columns for *each* input row (even though they are constants)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3235124272
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
Review Comment:
Yeah I'll add those now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-5114640585
I agree -- thanks again @HairstonE and @neilconway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-5094522548
@neilconway I made both those changes you suggested. Should be ready to merge!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3508066505
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
@@ -64,23 +66,52 @@ impl OptimizerRule for EliminateGroupByConstant {
.group_expr
.iter()
.partition(|expr| is_redundant_group_expr(expr, &group_by_columns));
-
if redundant.is_empty()
|| (required.is_empty() && aggregate.aggr_expr.is_empty())
{
return Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
}
- let simplified_aggregate = LogicalPlan::Aggregate(Aggregate::try_new(
- aggregate.input,
- required.into_iter().cloned().collect(),
- aggregate.aggr_expr.clone(),
- )?);
-
- let projection_expr =
- aggregate.group_expr.into_iter().chain(aggregate.aggr_expr);
+ let simplified_plan = if required.is_empty() {
+ let Some(registry) = config.function_registry() else {
+ return Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
+ };
+ let sentinel_name =
+ config.alias_generator().next("__row_count_sentinel");
+ let count_udaf = registry.udaf("count")?;
Review Comment:
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)
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,26 +60,42 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
Review Comment:
these plans are changing but I don't see the actual query results changing. Is that expected?
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
@@ -64,23 +66,52 @@ impl OptimizerRule for EliminateGroupByConstant {
.group_expr
.iter()
.partition(|expr| is_redundant_group_expr(expr, &group_by_columns));
-
if redundant.is_empty()
|| (required.is_empty() && aggregate.aggr_expr.is_empty())
{
return Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
}
- let simplified_aggregate = LogicalPlan::Aggregate(Aggregate::try_new(
- aggregate.input,
- required.into_iter().cloned().collect(),
- aggregate.aggr_expr.clone(),
- )?);
-
- let projection_expr =
- aggregate.group_expr.into_iter().chain(aggregate.aggr_expr);
+ let simplified_plan = if required.is_empty() {
+ let Some(registry) = config.function_registry() else {
+ return Ok(Transformed::no(LogicalPlan::Aggregate(aggregate)));
+ };
+ let sentinel_name =
+ config.alias_generator().next("__row_count_sentinel");
+ let count_udaf = registry.udaf("count")?;
+ let sentinel = count_udaf
+ .call(vec![lit(1_i64)])
+ .alias(sentinel_name.as_str());
+ let mut aggr_with_sentinel = aggregate.aggr_expr.clone();
Review Comment:
Can we avoid the clone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3312420726
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,10 +60,9 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
-01)Projection: Int64(123), Int64(456), Int64(789), count(Int64(1)), avg(t.c12)
-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)]]
Review Comment:
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 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-4982668995
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3658830367
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
@@ -64,10 +64,10 @@ impl OptimizerRule for EliminateGroupByConstant {
.group_expr
.iter()
.partition(|expr| is_redundant_group_expr(expr, &group_by_columns));
+ // 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.
Review Comment:
Slightly inaccurate: ungrouped COUNT returns a row with a 0, not a NULL. How about this:
```suggestion
// 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).
```
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
Review Comment:
Rename this test func so it doesn't imply that we eliminate constant grouping keys in this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-4547722172
This is on main:
```sql
> 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`:
```sql
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:
```shell
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3652876563
##########
datafusion/sqllogictest/test_files/optimizer_group_by_constant.slt:
##########
@@ -60,10 +60,9 @@ FROM test_table t
group by 1, 2, 3
----
logical_plan
-01)Projection: Int64(123), Int64(456), Int64(789), count(Int64(1)), avg(t.c12)
-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)]]
Review Comment:
Update here -- this is still a significantly worse plan, however, as @neilconway points out in https://github.com/apache/datafusion/pull/23871#issuecomment-5073629243 such a plan is likely not common (it produces a single group) so this is probably ok to change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "HairstonE (via GitHub)" <gi...@apache.org>.
HairstonE commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-4974936900
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: eliminate group by constant empty input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#issuecomment-4981215699
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org