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

Optimizing Expand+Aggregate in sqls with many count distinct #10798

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Optimizing Expand+Aggregate in sqls with many count distinct #10798

merged 6 commits into from
Sep 24, 2024

Conversation

binmahone
Copy link
Collaborator

@binmahone binmahone commented May 13, 2024 •
edited by winningsix
Loading

Fixing #10799. This PR tries to optimize the Expand&Aggregate exec in the first stage of a sql with many count distinct measures.

The optimizations in this PR include:

  1. Avoid allocating&initializing large number of null vectors when doing Expand
  2. Try coaleasce expanded column batches before sending them to Aggregate

@binmahone binmahone changed the title optimzing Expand+Aggregate in sqlw with many count distinct optimzing Expand+Aggregate in sqlw with many count distinct [WIP] May 13, 2024
@binmahone
Copy link
Collaborator Author

build

@winningsix winningsix changed the title optimzing Expand+Aggregate in sqlw with many count distinct [WIP] Optimzing Expand+Aggregate in sqlw with many count distinct [WIP] May 13, 2024
@binmahone binmahone changed the title Optimzing Expand+Aggregate in sqlw with many count distinct [WIP] Optimzing Expand+Aggregate in sqls with many count distinct [WIP] May 13, 2024
@sameerz sameerz added the performance A performance related task/issue label May 13, 2024
@binmahone
optimzing Expand+Aggregate in sqlw with many count distinct
cdc867d 
Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
@binmahone binmahone force-pushed the 240513_optimize_expand branch from d4a8261 to cdc867d Compare May 14, 2024 03:54
@binmahone
Copy link
Collaborator Author

build

@binmahone
Copy link
Collaborator Author

@revans2 @abellina @winningsix can you pls take a look of this PR ? we're going to pack a debug build based on this PR

revans2
revans2 requested changes May 14, 2024
View reviewed changes
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated
case class NullVecKey(d: DataType, n: Int)

class NullVecCache(private val maxNulls: Int)
extends util.LinkedHashMap[NullVecKey, GpuColumnVector](100, 0.75f, true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't understand why we are extending a map instead of wrapping it? Or even better using some other cache data structure built for this type of use case.

If we wrapped it, then we could get true LRU functionality and be able to reset the the priority on a read. It would let us not need to override remove so it throws. That API would just not exist.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated
// This is only for ExpandExec which will generate a lot of null vectors
case class NullVecKey(d: DataType, n: Int)

class NullVecCache(private val maxNulls: Int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The data stored in the cache needs to be spillable in some form. Eventually it would be nice to make it so instead of spilling we can just delete the value from the cache, but in the short term we need to make sure that everything stored in the cache is spillable.

It would also be really nice to have a timeout of some kind. If an entry is unused for a specific amount of time it should be deleted to avoid adding more memory pressure to the system.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala Outdated Show resolved Hide resolved
@binmahone binmahone mentioned this pull request May 27, 2024
Merged
@sameerz
Copy link
Collaborator

sameerz commented May 29, 2024

Please retarget to 24.08

@binmahone binmahone changed the base branch from branch-24.06 to branch-24.08 June 3, 2024 02:16
@binmahone binmahone mentioned this pull request Jun 17, 2024
Merged
@binmahone
Copy link
Collaborator Author

build

@binmahone
Copy link
Collaborator Author

@wjxiz1992 query perf pass

@binmahone
Copy link
Collaborator Author

binmahone commented Jul 2, 2024 •
edited
Loading

Hi @revans2 @abellina , since we're getting often-contradictory conclusions from customer side, we decide to hold on this PR until things are clearer. I'll turn back to address your comments once we're confident that these optimizations are always benificial.

@binmahone
Copy link
Collaborator Author

Hi @revans2 @abellina , since we're getting often-contradictory conclusions from customer side, we decide to hold on this PR until things are clearer. I'll turn back to address your comments once we're confident that these optimizations are always benificial.

@GaryShen2008 , I suggest to move this PR to 2410 because of the quoted reason

@sameerz
Copy link
Collaborator

sameerz commented Jul 29, 2024

Please retarget to the 24.10 branch.

@binmahone binmahone changed the base branch from branch-24.08 to branch-24.10 August 6, 2024 05:28
@binmahone
simplify
55eb7bd 
Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
@binmahone
Merge remote-tracking branch 'origin/branch-24.10' into 240513_optimi…
6f39db6 
…ze_expand

Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
@binmahone
add comment
2984563 
Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
@binmahone
Copy link
Collaborator Author

Hi @revans2 I simplified the code to make it unnecessary to worry about the side effects of global caching for null vectors. The cache reuse ratio would be smaller than previous version, but it would suffice for our customer's use case (a query with a lot of count distincts). Please help to review again

revans2
revans2 reviewed Sep 6, 2024
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

I have a few nits, but I think there are still some comments from @abellina that need to be addressed

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/basicPhysicalOperators.scala
val newColumns = boundExprs.safeMap(_.columnarEval(cb)).toArray[ColumnVector]
new ColumnarBatch(newColumns, cb.numRows())
try {
GpuExpressionsUtils.cachedNullVectors.get.clear()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are more ways than just this method to run a expression. I don't trust this to fix it every time. It is probably god enough in practice, but I don't like the precedence it is setting. At a minimum I want to see some comments here explaining what is happening and why. Preferably with a follow on issue to fix this once we have the ability to delete a buffer from the spill framework instead of spilling it.

@binmahone
address comments
31249fb 
Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
@binmahone binmahone changed the title Optimzing Expand+Aggregate in sqls with many count distinct [WIP] Optimzing Expand+Aggregate in sqls with many count distinct Sep 23, 2024
@abellina
Copy link
Collaborator

build

2 similar comments
@revans2
Copy link
Collaborator

revans2 commented Sep 23, 2024

build

@binmahone
Copy link
Collaborator Author

build

@binmahone binmahone merged commit 5725e2e into NVIDIA:branch-24.10 Sep 24, 2024
44 of 45 checks passed
@binmahone
Copy link
Collaborator Author

binmahone commented Sep 26, 2024 •
edited
Loading

close #10799

@sameerz sameerz changed the title Optimzing Expand+Aggregate in sqls with many count distinct Optimizing Expand+Aggregate in sqls with many count distinct Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abellina abellina abellina approved these changes

@revans2 revans2 revans2 approved these changes

Assignees
No one assigned
Labels
performance A performance related task/issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@binmahone @sameerz @abellina @revans2