You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "holdenk (via GitHub)" <gi...@apache.org> on 2026/08/12 19:09:09 UTC
[PR] Spark 58627 mark raise error throwable [spark]
holdenk opened a new pull request, #57975:
URL: https://github.com/apache/spark/pull/57975
### What changes were proposed in this pull request?
Corrects the `Expression.throwable` metadata on two expressions:
1. Overrides `throwable` to `true` on `RaiseError`.
2. Makes `Sequence`'s existing override fall back to its children rather than discarding the inherited default.
`Expression.throwable` (added in SPARK-46707) is opt-in metadata that tells the optimizer an expression may raise a runtime error, so a predicate containing it must not be relocated to a position where it runs on rows the original plan would not have evaluated it on. `RaiseError` always throws when evaluated but never declared the flag, so it inherited `children.exists(_.throwable)` -- false for the usual case of a literal error class and parameters.
`Sequence` was the only expression in the tree overriding the flag, and it did so as `stepOpt.isDefined`, dropping the inherited `children.exists(_.throwable)` term entirely. A throwing child under a stepless `sequence(...)` therefore reported non-throwable, which would also have masked the new flag on `RaiseError`.
### Why are the changes needed?
Without the flag, `CombineFilters` and `PushPredicateThroughJoin` treat a predicate containing `raise_error` (or `assert_true`, which is rewritten to `If(cond, null, RaiseError(...))`) as freely movable. Pushing such a predicate below a selective join, or merging it into a filter that would have removed the offending rows, can make a query fail at runtime that previously succeeded.
The join below matches no rows, so the predicate should never be evaluated. But the predicate has no column references, so `references.subsetOf(left.outputSet)` holds trivially and it is pushed onto the left side, where it fires on the first row of `t1`:
```sql
CREATE OR REPLACE TEMP VIEW t1 AS SELECT * FROM VALUES (1), (2), (3) AS t(a);
CREATE OR REPLACE TEMP VIEW t2 AS SELECT * FROM VALUES (4), (5), (6) AS t(b);
SELECT * FROM t1 JOIN t2 ON t1.a = t2.b WHERE raise_error('boom') IS NULL;
-- [USER_RAISED_EXCEPTION] boom SQLSTATE: P0001
```
With this change the query returns an empty result.
### Does this PR introduce _any_ user-facing change?
Yes, as a bug fix. A predicate containing `raise_error` or `assert_true` is no longer pushed through a join, pushed into a join condition, or combined with an adjacent filter, so the error is raised only on the rows the unoptimized plan would have evaluated it on. Queries that previously failed spuriously now succeed. The same now holds for a throwing expression nested under a stepless `sequence(...)`.
### How was this patch tested?
Added UTs:
- `MiscExpressionsSuite`: asserts the flag on `RaiseError`
- `FilterPushdownSuite`: a `raise_error` predicate is not pushed through a join and not combined with an adjacent filter, each paired with a non-throwing predicate of the same shape that still is.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5) human review and cleanup after by Holden
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58627][SQL] Mark raise error Throwable & fix sequence throwable [spark]
Posted by "asf-gitbox-commits (via GitHub)" <gi...@apache.org>.
asf-gitbox-commits closed pull request #57975: [SPARK-58627][SQL] Mark raise error Throwable & fix sequence throwable
URL: https://github.com/apache/spark/pull/57975
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58627][SQL] Mark raise error Throwable & fix sequence throwable [spark]
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on code in PR #57975:
URL: https://github.com/apache/spark/pull/57975#discussion_r3817220326
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala:
##########
@@ -1645,6 +1645,43 @@ class FilterPushdownSuite extends PlanTest {
comparePlans(optimizedQueryWithoutStep, correctAnswer)
}
+ test("SPARK-58627: do not push down predicate with a throwing child of sequence through joins") {
+ val x = testStringRelation.subquery("x")
+ val y = testRelation1.subquery("y")
+
+ // Sequence overrides `throwable` for its step check, so it also has to fall back to its
+ // children. Without that fallback a RaiseError under a stepless sequence reports
+ // non-throwable and the predicate gets pushed below the join.
+ val raiseErrorInt = RaiseError(
+ Literal("USER_RAISED_EXCEPTION"),
+ CreateMap(Seq(Literal("errorMessage"), $"x.e")),
+ IntegerType)
+ val queryWithRaiseError = x.join(y, joinType = Inner, condition = Some($"x.a" === $"y.d"))
+ .where(IsNotNull(Sequence($"x.a", raiseErrorInt, None)))
+ .analyze
+ comparePlans(Optimize.execute(queryWithRaiseError), queryWithRaiseError)
+ }
+
+ test("SPARK-58627: do not combine predicate with raise_error with other filters") {
+ val x = testStringRelation.subquery("x")
+
+ // Do not combine. Two stacked Filters pin raise_error above the inner predicate, while a
+ // single merged And does not: execution does not guarantee the conjuncts are evaluated in
+ // order, and later rules are free to re-split and relocate them independently. Either way
+ // raise_error can end up evaluated on rows the inner filter would have removed.
+ val queryWithRaiseError = x.where($"x.a" > 1)
+ .where(IsNull(RaiseError($"x.e")))
+ .analyze
+ comparePlans(Optimize.execute(queryWithRaiseError), queryWithRaiseError)
+
+ // The same shape without raise_error is combined into a single filter.
+ val queryWithoutRaiseError = x.where($"x.a" > 1)
+ .where(IsNotNull($"x.e"))
+ .analyze
+ val correctAnswer = x.where(IsNotNull($"x.e") && $"x.a" > 1).analyze
+ comparePlans(Optimize.execute(queryWithoutRaiseError), correctAnswer)
+ }
+
Review Comment:
Good catch, added an explicit simple test for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58627][SQL] Mark raise error Throwable & fix sequence throwable [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #57975:
URL: https://github.com/apache/spark/pull/57975#discussion_r3791448862
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala:
##########
@@ -1645,6 +1645,43 @@ class FilterPushdownSuite extends PlanTest {
comparePlans(optimizedQueryWithoutStep, correctAnswer)
}
+ test("SPARK-58627: do not push down predicate with a throwing child of sequence through joins") {
+ val x = testStringRelation.subquery("x")
+ val y = testRelation1.subquery("y")
+
+ // Sequence overrides `throwable` for its step check, so it also has to fall back to its
+ // children. Without that fallback a RaiseError under a stepless sequence reports
+ // non-throwable and the predicate gets pushed below the join.
+ val raiseErrorInt = RaiseError(
+ Literal("USER_RAISED_EXCEPTION"),
+ CreateMap(Seq(Literal("errorMessage"), $"x.e")),
+ IntegerType)
+ val queryWithRaiseError = x.join(y, joinType = Inner, condition = Some($"x.a" === $"y.d"))
+ .where(IsNotNull(Sequence($"x.a", raiseErrorInt, None)))
+ .analyze
+ comparePlans(Optimize.execute(queryWithRaiseError), queryWithRaiseError)
+ }
+
+ test("SPARK-58627: do not combine predicate with raise_error with other filters") {
+ val x = testStringRelation.subquery("x")
+
+ // Do not combine. Two stacked Filters pin raise_error above the inner predicate, while a
+ // single merged And does not: execution does not guarantee the conjuncts are evaluated in
+ // order, and later rules are free to re-split and relocate them independently. Either way
+ // raise_error can end up evaluated on rows the inner filter would have removed.
+ val queryWithRaiseError = x.where($"x.a" > 1)
+ .where(IsNull(RaiseError($"x.e")))
+ .analyze
+ comparePlans(Optimize.execute(queryWithRaiseError), queryWithRaiseError)
+
+ // The same shape without raise_error is combined into a single filter.
+ val queryWithoutRaiseError = x.where($"x.a" > 1)
+ .where(IsNotNull($"x.e"))
+ .analyze
+ val correctAnswer = x.where(IsNotNull($"x.e") && $"x.a" > 1).analyze
+ comparePlans(Optimize.execute(queryWithoutRaiseError), correctAnswer)
+ }
+
Review Comment:
The PR motivating example (WHERE raise_error('boom') IS NULL over a join) has no corresponding unit test. The two new tests cover CombineFilters and a Sequence-wrapped path through PushPredicateThroughJoin, but not the direct RaiseError in a join-predicate scenario. A bare RaiseError(Literal(...)) has no column references, so pre-fix references.subsetOf(left.outputSet) is vacuously true and the predicate is pushed; after the fix throwable=true blocks it. This case should be explicitly tested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org