[GLUTEN-8227][VL] fix: Update sort elimination rules for Hash Aggregate#9473
[GLUTEN-8227][VL] fix: Update sort elimination rules for Hash Aggregate#9473acvictor wants to merge 6 commits intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI on x86 |
8 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
acvictor
changed the title
|
@zhztheplayer @NEUpanning can you please help review this PR? |
|
@ulysses-you can you please take a look at this PR? |
|
Compared to having this flag, can we just add a new operator for offloaded sort aggregation? |
Thanks for the suggestion. I will try it out. |
acvictor
force-pushed
the
acvictor/sortRules
branch
from
60698f9 to
9a71c14
Compare
|
Run Gluten Clickhouse CI on x86 |
5 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
@zhztheplayer Any more comments? |
|
Apologize for the delay here! I'll revisit this shortly. Please feel free to jump in and review if anyone likes to. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
acvictor
force-pushed
the
acvictor/sortRules
branch
from
d1d3917 to
c600795
Compare
|
Run Gluten Clickhouse CI on x86 |
backends-velox/src/main/scala/org/apache/gluten/extension/FlushableHashAggregateRule.scala
Show resolved
Hide resolved
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
@zhztheplayer @rui-mo can you please review this again? The main change was in response to the comment "how about directly adding SortAggregateExecTransformer? Then we can decide whether to remove the sort based on the operator class." |
|
Run Gluten Clickhouse CI on x86 |
4 participants
What changes were proposed in this pull request?
Update sort elimination rules for hash aggregate only when the base aggregate was a sort aggregate. Only if a sort aggregate is transformed into a hash aggregate, can we safely assume that the aggregation does not depend on input order
(Part of fix for: #8227)
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)