-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
[SPARK-39447][SQL][3.2] Avoid AssertionError in AdaptiveSparkPlanExec.doExecuteBroadcast #37087
Conversation
@cloud-fan oh, this patch is based on #36953 but it does not backport into branch-3.2. Shall we also backport #36953 into branch-3.2 ? |
@@ -1597,6 +1597,25 @@ class DynamicPartitionPruningV1SuiteAEOff extends DynamicPartitionPruningV1Suite | |||
class DynamicPartitionPruningV1SuiteAEOn extends DynamicPartitionPruningV1Suite | |||
with EnableAdaptiveExecutionSuite { | |||
|
|||
test("SPARK-39447: Avoid AssertionError in AdaptiveSparkPlanExec.doExecuteBroadcast") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean this test case doesn't fail currently in branch-3.2 because we don't have SPARK-39551, @ulysses-you ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun sorry I made it not clear. I mean some tests failed in this pr due to we miss #36953 in branch-3.2. That said, this fix must work together with that pr.
+1, can you help to create the PR? |
Yep, +1 for backporting that PR first and revisit this PR after merging it. |
Since @ulysses-you seems to be busy, I made a backporting PR of #36953 , @cloud-fan , for Apache Spark 3.2.2 preparation. |
0147f6e
to
f011d7c
Compare
Thank you for rebasing, @ulysses-you . |
all test passed, the report seems broken |
thanks, merging to 3.2! |
….doExecuteBroadcast This is a backport of #36974 for branch-3.2 ### What changes were proposed in this pull request? Change `currentPhysicalPlan` to `inputPlan ` when we restore the broadcast exchange for DPP. ### Why are the changes needed? The currentPhysicalPlan can be wrapped with broadcast query stage so it is not safe to match it. For example: The broadcast exchange which is added by DPP is running before than the normal broadcast exchange(e.g. introduced by join). ### Does this PR introduce _any_ user-facing change? yes bug fix ### How was this patch tested? add test Closes #37087 from ulysses-you/inputplan-3.2. Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Thank you, @cloud-fan and @ulysses-you . |
….doExecuteBroadcast This is a backport of apache#36974 for branch-3.2 ### What changes were proposed in this pull request? Change `currentPhysicalPlan` to `inputPlan ` when we restore the broadcast exchange for DPP. ### Why are the changes needed? The currentPhysicalPlan can be wrapped with broadcast query stage so it is not safe to match it. For example: The broadcast exchange which is added by DPP is running before than the normal broadcast exchange(e.g. introduced by join). ### Does this PR introduce _any_ user-facing change? yes bug fix ### How was this patch tested? add test Closes apache#37087 from ulysses-you/inputplan-3.2. Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 32aff86) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
This is a backport of #36974 for branch-3.2
What changes were proposed in this pull request?
Change
currentPhysicalPlan
toinputPlan
when we restore the broadcast exchange for DPP.Why are the changes needed?
The currentPhysicalPlan can be wrapped with broadcast query stage so it is not safe to match it. For example:
The broadcast exchange which is added by DPP is running before than the normal broadcast exchange(e.g. introduced by join).
Does this PR introduce any user-facing change?
yes bug fix
How was this patch tested?
add test