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

Switch and/or to use new cudf binops to improve performance #4501

Merged
merged 4 commits into from
Jan 21, 2022

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Jan 11, 2022

This depends on rapidsai/cudf#10016

Seeing some good performance improvements, but need to run some more tests.

Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
@revans2 revans2 added the performance A performance related task/issue label Jan 11, 2022
@revans2 revans2 added this to the Jan 10 - Jan 28 milestone Jan 11, 2022
@revans2 revans2 self-assigned this Jan 11, 2022
Copy link
Collaborator

@abellina abellina left a comment

Choose a reason for hiding this comment

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

looks good (other than copyright)

Copy link
Contributor

@jbrennan333 jbrennan333 left a comment

Choose a reason for hiding this comment

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

Looks good to me as well.

@jlowe
Copy link
Member

jlowe commented Jan 11, 2022

I forgot there are cases where code was using the AST version of the NULL_LOGICAL_OR operator, see https://github.com/NVIDIA/spark-rapids/blob/branch-22.02/sql-plugin/src/main/scala/com/nvidia/spark/rapids/conditionalExpressions.scala#L445-L451. It would be good to have that code switch to using this new direct binop instead.

@revans2
Copy link
Collaborator Author

revans2 commented Jan 11, 2022

I ran the tests manually and fixed an issue with AST not being translated properly because it was separate from the normal code. That is fixed now.

I ran TPC-DS at scale factor 3k and it looks to be generally a win, 1.1% faster. But there are about as many tests that are slower 51 as there are that are faster 54. I am going to see if I can reproduce some of the slowness locally to try and dig in a little deeper.

@revans2
Copy link
Collaborator Author

revans2 commented Jan 20, 2022

Dependency just got merged into cudf. I'll move this out of draft when the dependency goes through CI

@revans2 revans2 marked this pull request as ready for review January 20, 2022 23:28
@revans2
Copy link
Collaborator Author

revans2 commented Jan 20, 2022

build

@revans2 revans2 merged commit 076521f into NVIDIA:branch-22.02 Jan 21, 2022
@revans2 revans2 deleted the and_perf_test branch January 21, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance A performance related task/issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants