[GLUTEN-4717][VL] Adapting the bind reference of agg that contains subquery in agg expressions #4705
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/oap-project/gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI |
|
@PHILO-HE Could you help to review? This bug causes some agg fallback. |
|
Before this PR: |
liujiayi771
force-pushed
the
subquery-in-agg
branch
from
3ab5108
to
0872183
Compare
|
Run Gluten Clickhouse CI |
liujiayi771
force-pushed
the
subquery-in-agg
branch
from
0872183
to
6c30e8b
Compare
|
Run Gluten Clickhouse CI |
liujiayi771
force-pushed
the
subquery-in-agg
branch
from
6c30e8b
to
4bc0a11
Compare
|
Run Gluten Clickhouse CI |
|
Seems there are some relevant test failure. Please fix it. Thanks! |
|
Run Gluten Clickhouse CI |
I just switched to using index, can you help review the latest code? |
These failed UTs include some other scenarios that need to be addressed. The current code has been modified to be more comprehensive, and these failed UTs used to be fallback. |
| // exprId. | ||
| val attrsWithSameName = originalInputAttributes.collect { | ||
| case a if a.name == attr.name => a | ||
| } |
There was a problem hiding this comment.
What happens if originalInputAttributes (grouping keys) has an attribute naing sum,count, etc ? Can we add a test case for that case ?
There was a problem hiding this comment.
This is a bad case; perhaps we should remove the group key in front of originalInputAttributes, because the output of the final agg's child is always the group key ++ inputAggBufferAttributes.
|
Run Gluten Clickhouse CI |
| |""".stripMargin)( | ||
| df => assert(getExecutedPlan(df).count(_.isInstanceOf[HashAggregateExecTransformer]) == 2)) | ||
|
|
||
| runQueryAndCompare(""" |
There was a problem hiding this comment.
@ulysses-you Add a new test case.
|
lgtm if test pass |
|
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
PHILO-HE
changed the title
|
Thanks for your great work! I just created an issue for possible reference in the future. |
What changes were proposed in this pull request?
When
aggregateExpressionsincludes a subquery, Spark'sPlanAdaptiveSubqueriesRule will transform the subquery within the final aggregation. TheaggregateFunctionin theaggregateExpressionsof the final aggregation will be cloned, resulting in creating newaggregateFunctionobjects. TheinputAggBufferAttributeswill also generate newAttributeReferenceinstances with largerexprId, which leads to a failure in binding with the output of the partial aggregation. We need to adapt to this situation; when encountering a failure to bind, it is necessary to allow the binding ofinputAggBufferAttributewith the same name but differentexprId.Fixes #4717.
How was this patch tested?
Add a test case in
VeloxAggregateFunctionsSuite.