yadavay-amzn commented on code in PR #56927:
URL: https://github.com/apache/spark/pull/56927#discussion_r3526243103
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala:
##########
@@ -1703,6 +1710,18 @@ class TypeCoercionSuite extends TypeCoercionSuiteBase {
LessThan(Cast(date0301, TimestampType), timestamp0301000001))
}
+ test("SPARK-57806: NullType expression with side effects is preserved in
string promotion") {
+ // raise_error has NullType as its dataType. When compared with a string,
the old code
+ // would replace raise_error with Literal(null, StringType), silently
discarding the side
+ // effect. The rule must wrap any NullType expression in a Cast so that
side effects are
+ // preserved; the optimizer will later constant-fold plain null literals
if applicable.
+ val rule = TypeCoercion.PromoteStrings
+ val raiseError = RaiseError(Literal(""))
+ ruleTest(rule,
+ EqualTo(raiseError, Literal("")),
+ EqualTo(Cast(raiseError, StringType), Literal("")))
+ }
Review Comment:
Non-blocking: the added `ruleTest`s assert the `Cast` wrapping at the
analyzer level, but the bug this fixes is a *runtime* missing-error — a
plan-shape test would still pass if a later rule ever stripped the `Cast`.
Worth one end-to-end test that actually executes and asserts the error fires,
e.g.:
```scala
intercept[SparkRuntimeException] {
sql("SELECT * FROM VALUES (1) t(x) WHERE raise_error('boom') =
'x'").collect()
}
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercionSuite.scala:
##########
@@ -422,6 +422,13 @@ class AnsiTypeCoercionSuite extends TypeCoercionSuiteBase {
ruleTest(AnsiTypeCoercion.ImplicitTypeCasts,
NumericTypeUnaryExpression(Literal.create(null, NullType)),
NumericTypeUnaryExpression(Literal.create(null, DoubleType)))
+
+ // SPARK-57806: a NullType expression with side effects must be wrapped in
Cast, not replaced
+ // by a null literal, so that the side effect is preserved.
+ val raiseError = RaiseError(Literal(""))
+ ruleTest(AnsiTypeCoercion.ImplicitTypeCasts,
+ NumericTypeUnaryExpression(raiseError),
+ NumericTypeUnaryExpression(Cast(raiseError, DoubleType)))
Review Comment:
Nit: this suite covers the `ImplicitTypeCasts` path but not the
`AnsiStringPromotionTypeCoercion` BinaryOperator path this PR also changes —
the non-ANSI `TypeCoercionSuite` has the equivalent `PromoteStrings` test. A
symmetric ANSI `PromoteStrings` `ruleTest` with a `RaiseError` operand would
close the asymmetry.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
