-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-38333][SQL] [3.1]DPP cause DataSourceScanExec java.lang.NullPointer… #35662
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-38333][SQL] [3.1]DPP cause DataSourceScanExec java.lang.NullPointer… #35662
Conversation
|
Does this issue only exist in Spark 3.1? If yes, please add |
Yes, only exists in Spark 3.1.Updated |
Cloud you explain why the master/3.2 does not have this issue? |
It fix by SPARK-35798 and the code related to DPP ,in spark master/3.2 the sql will be translated to normal join, instead of DPP |
|
For a bug fix pr, we need to add at least one UT. The new UT should fail before this pr and passed after this pr, which also helps to ensure that the change of other pr in the future will not keep this fix. |
|
On the other hand, if we backport SPARK-35798 to branch-3.1, can this issue be solved? |
After backport SPARK-35798 , because relation is null |
It is hard to add a unit test,as it only happen in the runtime |
|
Can one of the admins verify this patch? |
|
cc @cloud-fan |
Good catch, we may add a small end-to-end test? |
Agree with @weixiuli +1 |
I will add a test later |
I think we need to find which pr fix this issue correctly. then backport to spark 3.1 is the best way? or the code path is not same between 3.1 and master? |
|
So we should add a new test case and then we can use the new case to find which patches need to be backport faster |
It fix by #32947 and the code related to DPP ,in spark master/3.2 the sql will be translated to normal join, instead of DPP. |
| // `PlanExpression` wraps query plan. To compare query plans of `PlanExpression` on executor, | ||
| // can cause error like NPE. | ||
| (expr.isInstanceOf[PlanExpression[_]] && TaskContext.get != null) | ||
| (expr.find(_.isInstanceOf[PlanExpression[_]]).isDefined && TaskContext.get != null) |
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.
I think we should fix it at the master branch as well, as the code does not match the comment.
We can also add a UT in SubexpressionEliminationSuite, which tests addExprTree directly.
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.
OK, i will open another pr to fix it at the master branch and add a UT
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.
unit test added
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.
@cloud-fan How should this pr title be named? Maybe this is a potential problem.
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 we have a master branch PR now?
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.
@monkeyboy123 we can replace expr.find(_.isInstanceOf[PlanExpression[_]]).isDefined with TreeNode.exists api now
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 we have a master branch PR now?
Sorry for late reply,i will open a pr right now.
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.
@monkeyboy123 we can replace
expr.find(_.isInstanceOf[PlanExpression[_]]).isDefinedwithTreeNode.existsapi now
ok
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 we have a master branch PR now?
done, new pr SPARK-38333
862edb4 to
9b3a11c
Compare
9b3a11c to
708a168
Compare
So I think we should also find out which pr it can't be translated to DPP? And backport to branch-3.1. Then we can fix current pr's issue in both master/3.2/3.1. WDYT? @LuciferYang @cloud-fan |
+1 agree with you |
…function in Executor ### What changes were proposed in this pull request? It is master branch pr [SPARK-38333](#35662) ### Why are the changes needed? Bug fix, it is potential issue. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #36012 from monkeyboy123/spark-38333. Authored-by: Dereck Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
…function in Executor ### What changes were proposed in this pull request? It is master branch pr [SPARK-38333](#35662) ### Why are the changes needed? Bug fix, it is potential issue. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #36012 from monkeyboy123/spark-38333. Authored-by: Dereck Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a40acd4) Signed-off-by: Wenchen Fan <[email protected]>
…function in Executor It is master branch pr [SPARK-38333](#35662) Bug fix, it is potential issue. No UT Closes #36012 from monkeyboy123/spark-38333. Authored-by: Dereck Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a40acd4) Signed-off-by: Wenchen Fan <[email protected]>
…function in Executor It is master branch pr [SPARK-38333](#35662) Bug fix, it is potential issue. No UT Closes #36012 from monkeyboy123/spark-38333. Authored-by: Dereck Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a40acd4) Signed-off-by: Wenchen Fan <[email protected]>
|
Thanks for review all of you |
…function in Executor It is master branch pr [SPARK-38333](apache#35662) Bug fix, it is potential issue. No UT Closes apache#36012 from monkeyboy123/spark-38333. Authored-by: Dereck Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a40acd4) Signed-off-by: Wenchen Fan <[email protected]>
What changes were proposed in this pull request?
To fix canonicalization NPE, maybe is a proof of spark-35742,just as SPARK-23731 says
Why are the changes needed?
It is a bug:
run this sql in yarn client mode,it will generate DynamicPruningExpression:
BTW, function: exists extends CodegenFallback.
The root cause is addExprTree funtion in EquivalentExpressions:
as DPP will contains expressions : DynamicPruningExpression(InSubqueryExec(value, broadcastValues, exprId),
then executor will compile code, NPE will appears.
so, we should iterator all children,
(expr.find(_.isInstanceOf[PlanExpression[_]]).isDefined && TaskContext.get != null),if PlanExpression found, such as InSubqueryExec, we should skip addExprTree, then NPE will disappears.
Does this PR introduce any user-facing change?
Yes,
before this pr:
NPE will throw, like this:
after this pr,
everything is ok
How was this patch tested?
New UT