[SPARK-38868][SQL] Don't propagate exceptions from filter predicate when optimizing outer joins #36230

bersprockets · 2022-04-17T00:16:07Z

What changes were proposed in this pull request?

Change EliminateOuterJoin#canFilterOutNull to return false when a where condition throws an exception.

Why are the changes needed?

Consider this query:

select *
from (select id, id as b from range(0, 10)) l
left outer join (select id, id + 1 as c from range(0, 10)) r
on l.id = r.id
where assert_true(c > 0) is null;

The query should succeed, but instead fails with

java.lang.RuntimeException: '(c#1L > cast(0 as bigint))' is not true!

This happens even though there is no row where c > 0 is false.

The EliminateOuterJoin rule checks if it can convert the outer join to a inner join based on the expression in the where clause, which in this case is

assert_true(c > 0) is null

EliminateOuterJoin#canFilterOutNull evaluates that expression with c set to null to see if the result is null or false. That rule doesn't expect the result to be a RuntimeException, but in this case it always is.

That is, the assertion is failing during optimization, not at run time.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New unit test.

bersprockets · 2022-04-17T02:00:27Z

...alyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OuterJoinEliminationSuite.scala

+    val message = Literal(UTF8String fromString("Bad value"), StringType)
+    val originalQuery =
+      x.join(y, LeftOuter, Option("x.a".attr === "y.d".attr))
+        .where(If("y.d".attr > 0, true, RaiseError(message)).isNull)


I didn't do AssertTrue here because RuntimeReplaceable doesn't evaluate, so I just put in what AssertTrue gets replaced with.

HyukjinKwon · 2022-04-17T23:53:12Z

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

@@ -144,6 +144,8 @@ object EliminateOuterJoin extends Rule[LogicalPlan] with PredicateHelper {
    val emptyRow = new GenericInternalRow(attributes.length)
    val boundE = BindReferences.bindReference(e, attributes)
    if (boundE.exists(_.isInstanceOf[Unevaluable])) return false
+    // don't evaluate an expression that might purposefully throw an exception
+    if (boundE.exists(_.isInstanceOf[RaiseError])) return false


In fact, we should filter out all expressions that do not inherit NoThrow (and mark all other no-throw expressions). But that's incomplete now. Given that it already optimized all other expressions before, I am fine with this band-aid fix.

HyukjinKwon · 2022-04-17T23:53:27Z

cc @cloud-fan FYI

cloud-fan · 2022-04-18T07:33:23Z

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

@@ -144,6 +144,8 @@ object EliminateOuterJoin extends Rule[LogicalPlan] with PredicateHelper {
    val emptyRow = new GenericInternalRow(attributes.length)
    val boundE = BindReferences.bindReference(e, attributes)
    if (boundE.exists(_.isInstanceOf[Unevaluable])) return false
+    // don't evaluate an expression that might purposefully throw an exception
+    if (boundE.exists(_.isInstanceOf[RaiseError])) return false
    val v = boundE.eval(emptyRow)


can we do a try-catch and swallow the error here? RaiseError doesn't match all the expressions that may fail during evaluation.

can we do a try-catch and swallow the error here?

Looking...

RaiseError doesn't match all the expressions that may fail during evaluation.

I see what you mean. This query also fails in EliminateOuterJoin (because you can't have a null key):

select * from (select id, id as b from range(0, 10)) l left outer join (select id, id + 1 as c from range(0, 10)) r on l.id = r.id where map(c, '2') is not null;

I will add a try/catch.

cloud-fan · 2022-04-18T07:33:40Z

cc @sigmod

cloud-fan · 2022-04-22T03:03:31Z

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

+      val v = boundE.eval(emptyRow)
+      v == null || v == false
+    } catch {
+      case e: Exception =>


Let' follow the convention and catch NonFatal(e).

cloud-fan · 2022-04-22T03:03:50Z

...alyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OuterJoinEliminationSuite.scala

+    val x = testRelation.subquery(Symbol("x"))
+    val y = testRelation1.subquery(Symbol("y"))
+
+    val message = Literal(UTF8String fromString("Bad value"), StringType)


Suggested change

val message = Literal(UTF8String fromString("Bad value"), StringType)

val message = Literal(UTF8String.fromString("Bad value"), StringType)

…hen optimizing outer joins ### What changes were proposed in this pull request? Change `EliminateOuterJoin#canFilterOutNull` to return `false` when a `where` condition throws an exception. ### Why are the changes needed? Consider this query: ``` select * from (select id, id as b from range(0, 10)) l left outer join (select id, id + 1 as c from range(0, 10)) r on l.id = r.id where assert_true(c > 0) is null; ``` The query should succeed, but instead fails with ``` java.lang.RuntimeException: '(c#1L > cast(0 as bigint))' is not true! ``` This happens even though there is no row where `c > 0` is false. The `EliminateOuterJoin` rule checks if it can convert the outer join to a inner join based on the expression in the where clause, which in this case is ``` assert_true(c > 0) is null ``` `EliminateOuterJoin#canFilterOutNull` evaluates that expression with `c` set to `null` to see if the result is `null` or `false`. That rule doesn't expect the result to be a `RuntimeException`, but in this case it always is. That is, the assertion is failing during optimization, not at run time. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test. Closes #36230 from bersprockets/outer_join_eval_assert_issue. Authored-by: Bruce Robbins <bersprockets@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e2930b8) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2022-04-25T05:50:20Z

thanks, merging to master/3.3!

cloud-fan · 2022-04-25T05:50:53Z

@bersprockets do you want to open backport PRs for 3.2/3.1?

bersprockets · 2022-04-25T15:04:01Z

do you want to open backport PRs for 3.2/3.1?

Will do, thanks!

…hen optimizing outer joins Change `EliminateOuterJoin#canFilterOutNull` to return `false` when a `where` condition throws an exception. Consider this query: ``` select * from (select id, id as b from range(0, 10)) l left outer join (select id, id + 1 as c from range(0, 10)) r on l.id = r.id where assert_true(c > 0) is null; ``` The query should succeed, but instead fails with ``` java.lang.RuntimeException: '(c#1L > cast(0 as bigint))' is not true! ``` This happens even though there is no row where `c > 0` is false. The `EliminateOuterJoin` rule checks if it can convert the outer join to a inner join based on the expression in the where clause, which in this case is ``` assert_true(c > 0) is null ``` `EliminateOuterJoin#canFilterOutNull` evaluates that expression with `c` set to `null` to see if the result is `null` or `false`. That rule doesn't expect the result to be a `RuntimeException`, but in this case it always is. That is, the assertion is failing during optimization, not at run time. No. New unit test. Closes apache#36230 from bersprockets/outer_join_eval_assert_issue. Authored-by: Bruce Robbins <bersprockets@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ate when optimizing outer joins Backport of #36230 ### What changes were proposed in this pull request? Change `EliminateOuterJoin#canFilterOutNull` to return `false` when a `where` condition throws an exception. ### Why are the changes needed? Consider this query: ``` select * from (select id, id as b from range(0, 10)) l left outer join (select id, id + 1 as c from range(0, 10)) r on l.id = r.id where assert_true(c > 0) is null; ``` The query should succeed, but instead fails with ``` java.lang.RuntimeException: '(c#1L > cast(0 as bigint))' is not true! ``` This happens even though there is no row where `c > 0` is false. The `EliminateOuterJoin` rule checks if it can convert the outer join to a inner join based on the expression in the where clause, which in this case is ``` assert_true(c > 0) is null ``` `EliminateOuterJoin#canFilterOutNull` evaluates that expression with `c` set to `null` to see if the result is `null` or `false`. That rule doesn't expect the result to be a `RuntimeException`, but in this case it always is. That is, the assertion is failing during optimization, not at run time. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test. Closes #36341 from bersprockets/outer_join_eval_assert_issue_32. Authored-by: Bruce Robbins <bersprockets@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ate when optimizing outer joins Backport of #36230 ### What changes were proposed in this pull request? Change `EliminateOuterJoin#canFilterOutNull` to return `false` when a `where` condition throws an exception. ### Why are the changes needed? Consider this query: ``` select * from (select id, id as b from range(0, 10)) l left outer join (select id, id + 1 as c from range(0, 10)) r on l.id = r.id where assert_true(c > 0) is null; ``` The query should succeed, but instead fails with ``` java.lang.RuntimeException: '(c#1L > cast(0 as bigint))' is not true! ``` This happens even though there is no row where `c > 0` is false. The `EliminateOuterJoin` rule checks if it can convert the outer join to a inner join based on the expression in the where clause, which in this case is ``` assert_true(c > 0) is null ``` `EliminateOuterJoin#canFilterOutNull` evaluates that expression with `c` set to `null` to see if the result is `null` or `false`. That rule doesn't expect the result to be a `RuntimeException`, but in this case it always is. That is, the assertion is failing during optimization, not at run time. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test. Closes #36341 from bersprockets/outer_join_eval_assert_issue_32. Authored-by: Bruce Robbins <bersprockets@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 3690c8c) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ate when optimizing outer joins Backport of apache#36230 ### What changes were proposed in this pull request? Change `EliminateOuterJoin#canFilterOutNull` to return `false` when a `where` condition throws an exception. ### Why are the changes needed? Consider this query: ``` select * from (select id, id as b from range(0, 10)) l left outer join (select id, id + 1 as c from range(0, 10)) r on l.id = r.id where assert_true(c > 0) is null; ``` The query should succeed, but instead fails with ``` java.lang.RuntimeException: '(c#1L > cast(0 as bigint))' is not true! ``` This happens even though there is no row where `c > 0` is false. The `EliminateOuterJoin` rule checks if it can convert the outer join to a inner join based on the expression in the where clause, which in this case is ``` assert_true(c > 0) is null ``` `EliminateOuterJoin#canFilterOutNull` evaluates that expression with `c` set to `null` to see if the result is `null` or `false`. That rule doesn't expect the result to be a `RuntimeException`, but in this case it always is. That is, the assertion is failing during optimization, not at run time. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test. Closes apache#36341 from bersprockets/outer_join_eval_assert_issue_32. Authored-by: Bruce Robbins <bersprockets@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 3690c8c)

bersprockets added 2 commits April 15, 2022 18:20

Add failing test

bc663a9

Proposed fix

f001e02

github-actions bot added the SQL label Apr 17, 2022

bersprockets commented Apr 17, 2022

View reviewed changes

HyukjinKwon reviewed Apr 17, 2022

View reviewed changes

cloud-fan reviewed Apr 18, 2022

View reviewed changes

Address review feedback

cb31331

bersprockets changed the title ~~[SPARK-38868][SQL] Avoid RaiseError exceptions while optimizing outer joins~~ [SPARK-38868][SQL] Don't propagate exceptions from filter predicate when optimizing outer joins Apr 21, 2022

cloud-fan reviewed Apr 22, 2022

View reviewed changes

cloud-fan approved these changes Apr 22, 2022

View reviewed changes

address review feedback

ffc174c

cloud-fan closed this in e2930b8 Apr 25, 2022

bersprockets mentioned this pull request Apr 25, 2022

[SPARK-38868][SQL][3.2] Don't propagate exceptions from filter predicate when optimizing outer joins #36341

Closed

bersprockets deleted the outer_join_eval_assert_issue branch August 10, 2022 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-38868][SQL] Don't propagate exceptions from filter predicate when optimizing outer joins #36230

[SPARK-38868][SQL] Don't propagate exceptions from filter predicate when optimizing outer joins #36230

bersprockets commented Apr 17, 2022 •

edited

Loading

bersprockets Apr 17, 2022

HyukjinKwon Apr 17, 2022

HyukjinKwon commented Apr 17, 2022

cloud-fan Apr 18, 2022

bersprockets Apr 19, 2022

bersprockets Apr 21, 2022

cloud-fan commented Apr 18, 2022

cloud-fan Apr 22, 2022

cloud-fan Apr 22, 2022

cloud-fan commented Apr 25, 2022

cloud-fan commented Apr 25, 2022 •

edited

Loading

bersprockets commented Apr 25, 2022

	val message = Literal(UTF8String fromString("Bad value"), StringType)
	val message = Literal(UTF8String.fromString("Bad value"), StringType)

[SPARK-38868][SQL] Don't propagate exceptions from filter predicate when optimizing outer joins #36230

[SPARK-38868][SQL] Don't propagate exceptions from filter predicate when optimizing outer joins #36230

Conversation

bersprockets commented Apr 17, 2022 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

bersprockets Apr 17, 2022

Choose a reason for hiding this comment

HyukjinKwon Apr 17, 2022

Choose a reason for hiding this comment

HyukjinKwon commented Apr 17, 2022

cloud-fan Apr 18, 2022

Choose a reason for hiding this comment

bersprockets Apr 19, 2022

Choose a reason for hiding this comment

bersprockets Apr 21, 2022

Choose a reason for hiding this comment

cloud-fan commented Apr 18, 2022

cloud-fan Apr 22, 2022

Choose a reason for hiding this comment

cloud-fan Apr 22, 2022

Choose a reason for hiding this comment

cloud-fan commented Apr 25, 2022

cloud-fan commented Apr 25, 2022 • edited Loading

bersprockets commented Apr 25, 2022

bersprockets commented Apr 17, 2022 •

edited

Loading

cloud-fan commented Apr 25, 2022 •

edited

Loading