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

Support GpuHashJoin on Structs #2173

Merged
merged 9 commits into from
May 3, 2021
Merged

Support GpuHashJoin on Structs #2173

merged 9 commits into from
May 3, 2021

Conversation

sperlingxx
Copy link
Collaborator

@sperlingxx sperlingxx commented Apr 19, 2021 •
edited
Loading

Signed-off-by: sperlingxx [email protected]

Current PR is to partially support GPU equal join (GpuHashJoin) on structure data (#2126). There are some constraints in current PR:

  1. Due to some unresolved problems, we only support four join types: inner, left outer, right outer, cross, left anti and left semi. Left anti and left semi join on structs have yet supported in libcudf (#7912). Full outer is not supported because of #7934 and #7947.
  2. We can not support structs with float types, because Literal of struct type is not supported under GPU. [FEA] support GpuHashJoin on structs containing floats #2140
  3. Sort merge join on nested structs are not supported, because SortExec is not supported with nested structs.
  4. Current PR doesn't work with nullable structs until the cuDF fix PR #7963 getting merged.

@sameerz sameerz added the feature request New feature or request label Apr 19, 2021
@sperlingxx
some fix
c714489 
Signed-off-by: sperlingxx <[email protected]>
@revans2
Copy link
Collaborator

revans2 commented Apr 21, 2021

Looks really good just the one nit and then we can merge this in once the final fix is in place on the CUDF side.

@sperlingxx
some updates
f5ad331 
Signed-off-by: sperlingxx <[email protected]>
@sperlingxx
Copy link
Collaborator Author

build

@sperlingxx sperlingxx changed the title [WIP] support GpuHashJoin on Structs Support GpuHashJoin on Structs Apr 26, 2021
@sperlingxx
Copy link
Collaborator Author

build

3 similar comments
@sperlingxx
Copy link
Collaborator Author

build

@sperlingxx
Copy link
Collaborator Author

build

@sperlingxx
Copy link
Collaborator Author

build

@sperlingxx
Copy link
Collaborator Author

Looks really good just the one nit and then we can merge this in once the final fix is in place on the CUDF side.

The fix on the CUDF side is done. And all checks of CI pipeline are happy.

@sperlingxx sperlingxx requested a review from revans2 April 28, 2021 01:13
revans2
revans2 approved these changes Apr 29, 2021
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 had one really minor style nit, I don't expect you to fix it. It is small enough I am just going to ignore it.

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/execution/GpuHashJoin.scala Outdated
val unSupportNonEqualCondition = () => if (condition.isDefined) {
meta.willNotWorkOnGpu(s"$joinType joins currently do not support conditions")
}
val unSupportStructKeys = () => if (keyDataTypes.exists(_.isInstanceOf[StructType])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

I personally find it much more readable to do a def instead of a val for functions like this.

def unSupportNonEqualCondition:
  ...

def unSupportStructKeys:
  ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@revans2
Copy link
Collaborator

revans2 commented Apr 29, 2021

You had to do the build 4 times to get it to pass. Why exactly did https://blossom.nvidia.com/sw-gpu-spark-jenkins/job/rapids_premerge-github/1454/ fail (with what appears to be a join issue) and what changed to make it pass? I don't see anything that changed from CI. Which makes me really nervous.

revans2
revans2 requested changes Apr 29, 2021
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.

Removing my approval until I understand what happened with CI

@sperlingxx
some fix
d50560a 
Signed-off-by: sperlingxx <[email protected]>
@sperlingxx
Copy link
Collaborator Author

You had to do the build 4 times to get it to pass. Why exactly did https://blossom.nvidia.com/sw-gpu-spark-jenkins/job/rapids_premerge-github/1454/ fail (with what appears to be a join issue) and what changed to make it pass? I don't see anything that changed from CI. Which makes me really nervous.

The CI failed several times due to this cuDF bug. I wasn't aware of this bug until I found the CI still failed after multiple attempts. (I just thought it is because the cuDF Jar hasn't been updated to latest master. Therefore, I tried to rerun CI hours later.) Then, I looked into the error log and found the problem.

@sperlingxx
Copy link
Collaborator Author

build

@sperlingxx sperlingxx requested a review from revans2 May 3, 2021 08:42
@revans2 revans2 merged commit 244dcb0 into NVIDIA:branch-0.6 May 3, 2021
@sperlingxx sperlingxx deleted the struct_eq_join branch May 5, 2021 03:37
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@sperlingxx
Support GpuHashJoin on Structs (NVIDIA#2173)
18c85a3 
Signed-off-by: sperlingxx <[email protected]>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@sperlingxx
Support GpuHashJoin on Structs (NVIDIA#2173)
32c4b73 
Signed-off-by: sperlingxx <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revans2 revans2 revans2 approved these changes

Assignees
No one assigned
Labels
feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sperlingxx @revans2 @sameerz