[SPARK-20359][SQL] Avoid unnecessary execution in EliminateOuterJoin optimization that can lead to NPE #17660

koertkuipers · 2017-04-17T20:00:40Z

Avoid necessary execution that can lead to NPE in EliminateOuterJoin and add test in DataFrameSuite to confirm NPE is no longer thrown

What changes were proposed in this pull request?

Change leftHasNonNullPredicate and rightHasNonNullPredicate to lazy so they are only executed when needed.

How was this patch tested?

Added test in DataFrameSuite that failed before this fix and now succeeds. Note that a test in catalyst project would be better but i am unsure how to do this.

Please review http://spark.apache.org/contributing.html before opening a pull request.

…firm NPE is no longer thrown

SparkQA · 2017-04-17T22:17:06Z

Test build #75863 has finished for PR 17660 at commit 5e2b828.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-04-17T22:23:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/joins.scala

    if (boundE.find(_.isInstanceOf[Unevaluable]).isDefined) return false
-    val v = boundE.eval(emptyRow)
+    val v = try {
+      boundE.eval(emptyRow)


Could you check whether there exists the other similar cases in the code base that could trigger NullPointerException?

yeah sure i can do a scan for similar problems

cloud-fan · 2017-04-18T03:44:27Z

I think the root problem is, in EliminateOuterJoin.buildNewJoinType, we always build leftHasNonNullPredicate and rightHasNonNullPredicate. If it's left join, only rightHasNonNullPredicate is used, and when building leftHasNonNullPredicate, we may pass null values to a UDF that is not supposed to run on null values.

cloud-fan · 2017-04-18T05:04:55Z

In general, we can't decide whether a UDF is null-propagate or not, so we can't catch NPE for Expression.eval blindly. When we call Expression.eval in optimizer, we should make sure we do not pass unexpected input like EliminateOuterJoin did.

koertkuipers · 2017-04-18T11:53:09Z

I see. let me check if making leftHasNonNullPredicate and rightHasNonNullPredicate lazy solves it then

…

On Apr 17, 2017 23:44, "Wenchen Fan" ***@***.***> wrote: I think the root problem is, in EliminateOuterJoin.buildNewJoinType, we always build leftHasNonNullPredicate and rightHasNonNullPredicate. If it's left join, only rightHasNonNullPredicate is used, and when building leftHasNonNullPredicate, we may pass null values to a UDF that is not supposed to run on null values. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17660 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAyIJHOCvy6X0JVDoID_H3cDGYMZKGLUks5rxDG4gaJpZM4M_gRb> .

koertkuipers · 2017-04-18T15:18:41Z

@cloud-fan switching to lazy vals to avoid these predicates being evaluated when they are not used seems to work.
so i think this is a better (more targeted) solution for now, and i removed my try/catch logic.

SparkQA · 2017-04-18T17:33:05Z

Test build #75902 has finished for PR 17660 at commit 975c850.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-04-19T02:40:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/joins.scala

-    val leftHasNonNullPredicate = leftConditions.exists(canFilterOutNull)
-    val rightHasNonNullPredicate = rightConditions.exists(canFilterOutNull)
+    lazy val leftHasNonNullPredicate = leftConditions.exists(canFilterOutNull)
+    lazy val rightHasNonNullPredicate = rightConditions.exists(canFilterOutNull)


instead of lazy val, we can inline the xxx.exists in the case statements.

nit: They are used three times in case statements. Inline version looks too verbose.

viirya · 2017-04-19T05:24:55Z

As we don't know the details of UDF, shall we skip UDF for canFilterOutNull and directly return false?

…optimization that can lead to NPE Avoid necessary execution that can lead to NPE in EliminateOuterJoin and add test in DataFrameSuite to confirm NPE is no longer thrown ## What changes were proposed in this pull request? Change leftHasNonNullPredicate and rightHasNonNullPredicate to lazy so they are only executed when needed. ## How was this patch tested? Added test in DataFrameSuite that failed before this fix and now succeeds. Note that a test in catalyst project would be better but i am unsure how to do this. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Koert Kuipers <[email protected]> Closes #17660 from koertkuipers/feat-catch-npe-in-eliminate-outer-join. (cherry picked from commit 608bf30) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2017-04-19T07:55:23Z

thanks, merging to master/2.2/2.1!

cloud-fan · 2017-04-19T07:56:52Z

As we don't know the details of UDF, shall we skip UDF for canFilterOutNull and directly return false?

Then we may miss some opportunities for optimization.

…optimization that can lead to NPE Avoid necessary execution that can lead to NPE in EliminateOuterJoin and add test in DataFrameSuite to confirm NPE is no longer thrown ## What changes were proposed in this pull request? Change leftHasNonNullPredicate and rightHasNonNullPredicate to lazy so they are only executed when needed. ## How was this patch tested? Added test in DataFrameSuite that failed before this fix and now succeeds. Note that a test in catalyst project would be better but i am unsure how to do this. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Koert Kuipers <[email protected]> Closes apache#17660 from koertkuipers/feat-catch-npe-in-eliminate-outer-join.

koertkuipers added 2 commits April 16, 2017 20:11

catch NPE in EliminateOuterJoin and add test in DataFrameSuite to con…

7a759cc

…firm NPE is no longer thrown

add actual jira

5e2b828

gatorsmile reviewed Apr 17, 2017

View reviewed changes

use lazy vals instead of catching NPE

975c850

koertkuipers changed the title ~~[SPARK-20359][SQL] catch NPE in EliminateOuterJoin optimization~~ [SPARK-20359][SQL] Avoid unnecessary execution in EliminateOuterJoin optimization that can lead to NPE Apr 18, 2017

cloud-fan reviewed Apr 19, 2017

View reviewed changes

asfgit closed this in 608bf30 Apr 19, 2017

[SPARK-20359][SQL] Avoid unnecessary execution in EliminateOuterJoin optimization that can lead to NPE #17660

[SPARK-20359][SQL] Avoid unnecessary execution in EliminateOuterJoin optimization that can lead to NPE #17660

Uh oh!

Conversation

koertkuipers commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 17, 2017

Uh oh!

gatorsmile Apr 17, 2017

Choose a reason for hiding this comment

Uh oh!

koertkuipers Apr 17, 2017

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 18, 2017

Uh oh!

cloud-fan commented Apr 18, 2017

Uh oh!

koertkuipers commented Apr 18, 2017 via email

Uh oh!

koertkuipers commented Apr 18, 2017

Uh oh!

SparkQA commented Apr 18, 2017

Uh oh!

cloud-fan Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

viirya Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Apr 19, 2017

Uh oh!

cloud-fan commented Apr 19, 2017

Uh oh!

cloud-fan commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

koertkuipers commented Apr 17, 2017 •

edited

Loading

viirya Apr 19, 2017 •

edited

Loading

cloud-fan commented Apr 19, 2017 •

edited

Loading