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.

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

[FEA] Optimize unnecessary sorts when replacing SortAggregate #601

Closed
jlowe opened this issue Aug 21, 2020 · 0 comments · Fixed by #686
Closed

[FEA] Optimize unnecessary sorts when replacing SortAggregate #601

jlowe opened this issue Aug 21, 2020 · 0 comments · Fixed by #686
Assignees
Labels
feature request New feature or request P1 Nice to have for release performance A performance related task/issue

Comments

@jlowe
Copy link
Member

jlowe commented Aug 21, 2020

Is your feature request related to a problem? Please describe.
I noticed a query that had a GPU plan with this sequence of nodes somewhere in it:
GpuSort --> GpuHashAggregate --> GpuColumnarExchange --> GpuSort --> GpuHashAggregate

GpuHashAggregate makes no assumptions on the order of its input, so the two sorts aren't accomplishing anything useful. It looks like the original CPU plan used a sort-based aggregate and the plugin blindly replaced the sort with GpuSort and SortAggregate with HashAggregate. If the plugin is going to use HashAggregate to replace SortAggregate then it should remove the unnecessary sorts.

Describe the solution you'd like
The plugin removes unnecessary sorts in the plan when replacing a sort aggregate with a hash aggregate. This is similar to the behavior the plugin already performs when replacing a SortMergeJoin with a GpuShuffledHashJoin.

Note that a sort may be required after the aggregate if subsequent nodes in the plan need the output ordering guaranteed by SortAggregate. We should be able to leverage the same "insert sorts when needed" logic that is being performed when replacing SortMergeJoin with GpuShuffledHashJoin.

Describe alternatives you've considered
We could try to leverage libcudf's sort-based aggregate functionality, but we probably only want to leverage it when we know the data happens to already be sorted on the grouping keys before the aggregate. I don't think we want to explicitly sort on the keys, as libcudf will already do this if necessary when performing an aggregate that must be performed using sorted data.

@jlowe jlowe added feature request New feature or request ? - Needs Triage Need team to review and classify performance A performance related task/issue labels Aug 21, 2020
@sameerz sameerz added P1 Nice to have for release and removed ? - Needs Triage Need team to review and classify labels Aug 25, 2020
@revans2 revans2 self-assigned this Sep 8, 2020
@revans2 revans2 added this to the Aug 31 - Sep 11 milestone Sep 8, 2020
tgravescs pushed a commit to tgravescs/spark-rapids that referenced this issue Nov 30, 2023
…IDIA#601)

Signed-off-by: spark-rapids automation <[email protected]>

Signed-off-by: spark-rapids automation <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request P1 Nice to have for release performance A performance related task/issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants