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

Fix issue with DPP and AQE on reused broadcast exchanges [databricks] #11118

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Fix issue with DPP and AQE on reused broadcast exchanges [databricks] #11118

merged 3 commits into from
Jul 3, 2024

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Jul 1, 2024

This adjusts how a reused broadcast is translated to avoid an assertion error.

@revans2 revans2 requested a review from NVnavkumar July 1, 2024 18:37
@revans2
Copy link
Collaborator Author

revans2 commented Jul 1, 2024

build

@sameerz sameerz added the bug Something isn't working label Jul 1, 2024
@revans2
Copy link
Collaborator Author

revans2 commented Jul 1, 2024

looks like it failed on Spark 3.1.1 with a filter not being on the GPU. I need to do some more investigation to see why that happened. It is likely an issue with the test, but I am not 100% sure.

@revans2
Copy link
Collaborator Author

revans2 commented Jul 1, 2024

build

@viadea
Copy link
Collaborator

viadea commented Jul 1, 2024

@revans2 should the title of the PR be for dataproc instead of databricks?

@NVnavkumar
Copy link
Collaborator

@revans2 should the title of the PR be for dataproc instead of databricks?

No, that's just a magic item for CI to test the fix on Databricks in addition to Apache Spark since the former is affected by this change.

@sameerz
Copy link
Collaborator

sameerz commented Jul 1, 2024

looks like it failed on Spark 3.1.1 with a filter not being on the GPU. I need to do some more investigation to see why that happened. It is likely an issue with the test, but I am not 100% sure.

It looks like you already fixed the test. We do not support Spark 3.1.x in 24.08+. Is pre-merge still testing Spark 3.1.1?

@sameerz
Copy link
Collaborator

sameerz commented Jul 1, 2024

looks like it failed on Spark 3.1.1 with a filter not being on the GPU. I need to do some more investigation to see why that happened. It is likely an issue with the test, but I am not 100% sure.

It looks like you already fixed the test. We do not support Spark 3.1.x in 24.08+. Is pre-merge still testing Spark 3.1.1?

Ah, I guess it is per #11041

NVnavkumar
NVnavkumar previously approved these changes Jul 2, 2024
View reviewed changes
Copy link
Collaborator

@NVnavkumar NVnavkumar left a comment

Choose a reason for hiding this comment

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

LGTM

jlowe
jlowe previously approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Minor comment nits, lgtm.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuTransitionOverrides.scala Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuTransitionOverrides.scala Outdated Show resolved Hide resolved
@revans2 revans2 dismissed stale reviews from jlowe and NVnavkumar via 115ec1b July 3, 2024 17:09
@revans2
Copy link
Collaborator Author

revans2 commented Jul 3, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Jul 3, 2024

@jlowe and @NVnavkumar please take another look.

@revans2 revans2 merged commit 5635fd4 into NVIDIA:branch-24.08 Jul 3, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NVnavkumar NVnavkumar NVnavkumar approved these changes

@jlowe jlowe jlowe approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@revans2 @viadea @NVnavkumar @sameerz @jlowe