You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "chenkovsky (via GitHub)" <gi...@apache.org> on 2025/05/23 07:57:23 UTC
[PR] feat: optimize and unparse grouping [datafusion]
chenkovsky opened a new pull request, #16161:
URL: https://github.com/apache/datafusion/pull/16161
## Which issue does this PR close?
## Rationale for this change
first, it seems that grouping udaf document is not correct.
and for aggregation with grouping,
e.g.
```sql
CREATE TABLE test (c1 VARCHAR,c2 VARCHAR,c3 INT) as values
('a','A',1), ('b','B',2);
EXPLAIN FORMAT INDENT select
c1,
c2,
CASE WHEN grouping(c1) = 1 THEN sum(c3) ELSE NULL END as gx,
grouping(c1) as g0,
grouping(c2) as g1,
grouping(c1, c2) as g2,
grouping(c2, c1) as g3
from
test
group by
grouping sets (
(c1, c2),
(c1),
(c2),
()
);
```
current logical plan is:
```
| logical_plan | Projection: test.c1, test.c2, CASE WHEN grouping(test.c1) = Int32(1) THEN sum(test.c3) ELSE Int64(NULL) END AS gx, grouping(test.c1) AS g0, grouping(test.c2) AS g1, grouping(test.c1,test.c2) AS g2, grouping(test.c2,test.c1) AS g3 |
| | Projection: test.c1, test.c2, CAST(__common_expr_1 AS Int32) AS grouping(test.c1), sum(test.c3), CAST(__common_expr_2 AS Int32) AS grouping(test.c2), CAST(__grouping_id AS Int32) AS grouping(test.c1,test.c2), CAST(__common_expr_1 | __common_expr_2 << UInt8(1) AS Int32) AS grouping(test.c2,test.c1) |
| | Projection: __grouping_id & UInt8(2) >> UInt8(1) AS __common_expr_1, __grouping_id & UInt8(1) AS __common_expr_2, test.c1, test.c2, __grouping_id, sum(test.c3) |
| | Aggregate: groupBy=[[GROUPING SETS ((test.c1, test.c2), (test.c1), (test.c2), ())]], aggr=[[sum(CAST(test.c3 AS Int64))]] |
| | TableScan: test projection=[c1, c2, c3]
```
the problems are:
1. there are three projections. for bitwise operation, there's no benifit for extra projection.
2. it makes grouping level optimization very hard. for example,
```sql
select case when grouping(c1) = 1 then sum(c2) else null end from test group by grouping sets (...)
```
we only need to calculate sum(c2) in when grouping(c1) = 1, this is a very useful optimization trick. I'm also using this trick to optimizing sql with multiple count distinct.
3. unparsing is not supported. Internal("Tried to unproject column referring to internal grouping id") will be thrown.
## What changes are included in this PR?
1. create a grouping udf.
2. modify analyzer, replace grouping_udaf with grouping udf
3. change it back, when unparsing.
now, the logical plan is:
```
| logical_plan | Projection: test.c1, test.c2, CASE WHEN grouping(test.c1) = Int32(1) THEN sum(test.c3) ELSE Int64(NULL) END AS gx, grouping(test.c1) AS g0, grouping(test.c2) AS g1, grouping(test.c1,test.c2) AS g2, grouping(test.c2,test.c1) AS g3 |
| | Projection: test.c1, test.c2, grouping(__grouping_id, List([1])) AS grouping(test.c1), sum(test.c3), grouping(__grouping_id, List([0])) AS grouping(test.c2), grouping(__grouping_id) AS grouping(test.c1,test.c2), grouping(__grouping_id, List([0, 1])) AS grouping(test.c2,test.c1) |
| | Aggregate: groupBy=[[GROUPING SETS ((test.c1, test.c2), (test.c1), (test.c2), ())]], aggr=[[sum(CAST(test.c3 AS Int64))]] |
| | TableScan: test projection=[c1, c2, c3]
```
there are only two projections. and unparsing is supported.
## Are these changes tested?
UT
## Are there any user-facing changes?
No
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "eejbyfeldt (via GitHub)" <gi...@apache.org>.
eejbyfeldt commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-2981365745
I did not look closely at yet since I have not really contributed here in months.
> there are three projections. for bitwise operation, there's no benifit for extra projection.
This extra projection is introduced by `CommonSubexprEliminate`. I understand that the changes make it so that they do not interact in the presented query, but is this really a general fix of that behavior?
But the goal of leaving more structure in logical plan after resolving the grouping expr so that optimization and unparsing sounds reasonable to me.
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3367555224
> Hi, I ran my code using this branch and unfortunately it did not solve my issue (https://github.com/apache/datafusion/issues/16590).
hi，could you please push your code， then i can see what's your issue. currently i havent touch substrait， some extra codes should be modified to solve issue 16590.
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#discussion_r2164876290
##########
datafusion/sql/Cargo.toml:
##########
@@ -48,6 +48,7 @@ arrow = { workspace = true }
bigdecimal = { workspace = true }
datafusion-common = { workspace = true, default-features = true }
datafusion-expr = { workspace = true }
+datafusion-functions-aggregate = { workspace = true }
Review Comment:
We have tried hard to avoid having the sql parser depend on function libraries -- is there any way to avoid changing this dependency?
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3289089870
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "Slimsammylim (via GitHub)" <gi...@apache.org>.
Slimsammylim commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3361517517
Hi @alamb and @chenkovsky! I am running into the same issue @lorenarosati brought up a couple months ago (https://github.com/apache/datafusion/issues/16590) and have not found a workaround solution yet. Just wanted to check in, what's the status of this PR?
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3297976536
Thanks @chenkovsky
@eejbyfeldt it looks like you contributed the initial implementation of `grouping` in https://github.com/apache/datafusion/pull/12704
Could you help review this PR?
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3289341290
keep it open
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3368161956
I added substrait support
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on code in PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#discussion_r2404342475
##########
datafusion/expr/src/grouping.rs:
##########
@@ -0,0 +1,119 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::{any::Any, sync::Arc};
+
+use arrow::{
+ array::{Array, Int32Array},
+ datatypes::DataType,
+};
+use datafusion_common::{internal_err, Result, ScalarValue};
+use datafusion_expr_common::{
+ accumulator::Accumulator,
+ signature::{Signature, Volatility},
+};
+use datafusion_functions_aggregate_common::accumulator::AccumulatorArgs;
+
+use crate::{
+ expr::{AggregateFunction, ScalarFunction},
+ utils::grouping_set_to_exprlist,
+ Aggregate, AggregateUDF, AggregateUDFImpl, Expr,
+};
+
+// To avoid adding datafusion-functions-aggregate dependency, implement a DummyGroupingUDAF here
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct DummyGroupingUDAF {
Review Comment:
currently, I created a dummy udaf. maybe we can move grouping udaf to datafusion-expr module directly. because grouping udaf is a pseudo udaf itself.
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "Slimsammylim (via GitHub)" <gi...@apache.org>.
Slimsammylim commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3371792240
Hi! I tried again and but I keep getting the same error message.
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "Slimsammylim (via GitHub)" <gi...@apache.org>.
Slimsammylim commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3372266976
Hi! I am not running SQL. The type of plan that is failing for me is the same as the one Victor mentions in https://github.com/apache/datafusion/pull/17909. Lorena mentions a similar plan in https://github.com/apache/datafusion/issues/16590.
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky closed pull request #16161: feat: optimize grouping and introduced unparsing and substrait support
URL: https://github.com/apache/datafusion/pull/16161
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3374775236
> Hi! I am not running SQL. The type of plan that is failing for me is the same as the one Victor mentions in #17909. Lorena mentions a similar plan in #16590.
@Slimsammylim I see, I only tested roundtrip, you only consume substrait plan. I think the simplest workaround is moving __grouping_id to the end of schema, then index will be correct. but It's too tricky. also there's another issue caused by __grouping_id #16983. It's better to remove __grouping_id.
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-2906801006
@eejbyfeldt could you please help me review this PR?
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-2906786682
Thanks @chenkovsky -- can. you find the original PR that added this `GROUPING` function and perhaps @ mention the author to see if they have any feedback / could help with review?
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3380889170
@Slimsammylim I created another PR. #17961 , please take a snapshot. it will not touch grouping udaf in logical plan level. so It will support substrait by design.
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3371817642
> Hi! I tried again and but I keep getting the same error message.
@Slimsammylim
hi, what's your sql, it's with grouping or without grouping? for no grouping, it seems that there's a PR #17909. this pr can solve multiple grouping issue.
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-2906804315
it's related to #12704
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3363067462
> Hi @alamb and @chenkovsky! I am running into the same issue @lorenarosati brought up a couple months ago (#16590) and have not found a workaround solution yet. Just wanted to check in, what's the status of this PR?
I am waiting for someone to help review the PR. Can you help here @Slimsammylim ? Perhaps verify that this solution fixes your issue?
--
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] feat: optimize and unparse grouping [datafusion]
Posted by "Slimsammylim (via GitHub)" <gi...@apache.org>.
Slimsammylim commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3366066625
Hi, I ran my code using this branch and unfortunately it did not solve my issue (https://github.com/apache/datafusion/issues/16590).
--
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] feat: optimize grouping and introduced unparsing and substrait support [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #16161:
URL: https://github.com/apache/datafusion/pull/16161#issuecomment-3419690898
In favour of #17961, close this
--
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