Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GLUTEN-7751][VL] Merge two consecutive aggregates to one in complete mode #7752

Merged
merged 1 commit into from
Nov 20, 2024
Merged

[GLUTEN-7751][VL] Merge two consecutive aggregates to one in complete mode #7752

merged 1 commit into from
Nov 20, 2024

Conversation

yikf
Copy link
Contributor

@yikf yikf commented Oct 31, 2024

What changes were proposed in this pull request?

Fix #7751.

In issue #4668, support for the CH backend's merge agg capability was added. We can also support this feature in the velox backend.

How was this patch tested?

Through existing unit tests and GA verification, and additional new unit tests have been added.

@github-actions github-actions bot added CORE works for Gluten Core VELOX labels Oct 31, 2024
@github-actions GitHub Actions
Copy link

#7751

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

1 similar comment
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@yikf
Copy link
Contributor Author

yikf commented Oct 31, 2024

@zzcclp @PHILO-HE Could you please take a look if you find a time, thanks

@zhouyuan
Copy link
Contributor

zhouyuan commented Nov 1, 2024

@yikf can you please also check if COMPLETE agg is offload to Velox native? I think we need more changes to support this feature

@FelixYBW
Copy link
Contributor

FelixYBW commented Nov 1, 2024

Glad to see we can learn each other from different backends. It's one of our goal to support different backends in Gluten.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 6, 2024

Run Gluten Clickhouse CI on x86

@github-actions github-actions bot added the DOCS label Nov 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 6, 2024

Run Gluten Clickhouse CI on x86

weixiuli
weixiuli reviewed Nov 7, 2024
View reviewed changes
docs/Configuration.md Outdated Show resolved Hide resolved
@jackylee-ch
Copy link
Contributor

jackylee-ch commented Nov 7, 2024 •
edited
Loading

@yikf can you please also check if COMPLETE agg is offload to Velox native? I think we need more changes to support this feature

@zhouyuan This pr works well in our environment with COMPLETE mode Agg, I think we can move on now.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Run Gluten Clickhouse CI on x86

PHILO-HE
PHILO-HE reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work!

...x/src/test/scala/org/apache/gluten/execution/VeloxMergeTwoPhasesHashBaseAggregateSuite.scala Outdated Show resolved Hide resolved
backends-velox/src/test/scala/org/apache/gluten/execution/VeloxAggregateFunctionsSuite.scala Show resolved Hide resolved
gluten-ut/spark33/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala Outdated Show resolved Hide resolved
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
...x/src/test/scala/org/apache/gluten/execution/VeloxMergeTwoPhasesHashBaseAggregateSuite.scala Outdated Show resolved Hide resolved
...x/src/test/scala/org/apache/gluten/execution/VeloxMergeTwoPhasesHashBaseAggregateSuite.scala Outdated Show resolved Hide resolved
...x/src/test/scala/org/apache/gluten/execution/VeloxMergeTwoPhasesHashBaseAggregateSuite.scala Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@PHILO-HE PHILO-HE changed the title [GLUTEN-7751][VL] Velox backend support merge two aggregate to one complete mode aggregate [GLUTEN-7751][VL] Merge two consecutive aggregates to one in complete mode Nov 13, 2024
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

1 similar comment
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

1 similar comment
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

zhztheplayer
zhztheplayer approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@zhztheplayer zhztheplayer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to add MergeTwoPhasesHashBaseAggregate to injectRas list the remove "spark.gluten.ras.enabled" -> "false" settings in UT code to make the feature covered by RAS UT? Which could be done in a separate PR. Thanks.

zhouyuan
zhouyuan approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@zhouyuan zhouyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
verified locally with a patched spark with pushing agg thru join enabled

@zhouyuan zhouyuan merged commit 5d7b963 into apache:main Nov 20, 2024
46 checks passed
@yikf yikf deleted the velox-merge-agg branch November 20, 2024 02:10
@zhztheplayer zhztheplayer mentioned this pull request Nov 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weixiuli weixiuli weixiuli left review comments

@PHILO-HE PHILO-HE PHILO-HE left review comments

@jackylee-ch jackylee-ch jackylee-ch approved these changes

@zzcclp zzcclp zzcclp left review comments

@zhztheplayer zhztheplayer zhztheplayer approved these changes

@zhouyuan zhouyuan zhouyuan approved these changes

Assignees
No one assigned
Labels
CORE works for Gluten Core DOCS VELOX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] Velox backend support merge two phase hash-based aggregate into one aggregate
8 participants
@yikf @zhouyuan @FelixYBW @jackylee-ch @zzcclp @zhztheplayer @PHILO-HE @weixiuli