wangyum commented on code in PR #59344:
URL: https://github.com/apache/spark/pull/59344#discussion_r4296406514
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -262,6 +264,9 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
private def canEvaluateUnconditionallyInternal(e: Expression): Boolean = e
match {
case _: AttributeReference | _: Literal => true
+ case Cast(child, toType, _, _) =>
+ child.dataType.isInstanceOf[NumericType] &&
toType.isInstanceOf[NumericType] &&
+ Cast.canUpCast(child.dataType, toType) &&
canEvaluateUnconditionallyInternal(child)
Review Comment:
Fixed in 7afd4fde9bc: the Cast arm now requires a decimal source to widen
only to a decimal target (`!child.dataType.isInstanceOf[DecimalType] ||
toType.isInstanceOf[DecimalType]`), so the overflowing pairs admitted by
`isTighterThan` are excluded. `isTighterThan` itself is left untouched since it
is shared with analyzer up-cast validation.
Added regressions in `ExprUtilsSuite` (`Cast(<DECIMAL(19,0) attr>,
LongType)` rejected) and at the split level in
`SplitStreamedSideJoinConditionSuite` (the conjunct stays in the rest), plus
positives for integral -> wider-decimal and decimal -> wider-decimal over
attributes, keeping the existing Int -> Long admission. Verified the new
assertions fail if the decimal restriction is dropped from the arm.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/joins/SplitStreamedSideJoinConditionSuite.scala:
##########
@@ -151,6 +163,15 @@ class SplitStreamedSideJoinConditionSuite extends
QueryTest with SharedSparkSess
|ON s.id = b.bid AND s.a < 5
""".stripMargin))
+ testWithCodegenOnAndOff(
+ "left outer join with lossless widening cast in streamed-only condition")(
+ spark.sql("""
+ |SELECT /*+ BROADCAST(b) */ s.id, s.a, s.pad, b.bid
+ |FROM (SELECT id, CAST(id AS int) AS a, pad FROM streamed_t WHERE pad %
2 = 0) s
+ |LEFT OUTER JOIN (SELECT DISTINCT bid FROM build_t) b
+ |ON s.id = b.bid AND CAST(s.a AS bigint) < 5L
Review Comment:
Fixed in 7afd4fde9bc: the end-to-end test now uses `CAST(s.id AS double) <
5.0D` (Long -> Double cannot be unwrapped per SPARK-39476), and from the
executed `BroadcastHashJoinExec` it re-runs the split the join performs on its
actual post-optimizer condition, asserting the Cast conjunct lands in the
streamed-only part while nothing stays in the rest.
Verified per the stated acceptance criteria: with the Cast arm of the
whitelist removed this assertion now fails with whole-stage codegen on and off,
whereas the previous Int -> Long version still passed; with the arm present,
rows are identical with hoisting enabled and disabled.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -243,14 +243,16 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
*
* Only a whitelist of total, deterministic expressions qualifies:
* - leaves: attribute references and literals;
+ * - lossless widening numeric casts: Cast where both types are numeric
and Cast.canUpCast
Review Comment:
Fixed in 7afd4fde9bc: the whitelist bullet now reads "non-throwing widening
numeric up-casts (Cast.canUpCast)" and explicitly states the admitted casts are
total but not necessarily lossless (Int/Long -> Float/Double can round), and
that what is relied on is the casts never throwing, not losslessness. The two
test names that said "lossless" were renamed accordingly.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExprUtilsSuite.scala:
##########
@@ -44,6 +46,10 @@ class ExprUtilsSuite extends SparkFunSuite {
assert(!arithmetic.throwable)
assert(!ExprUtils.canEvaluateUnconditionally(arithmetic))
assert(!ExprUtils.canEvaluateUnconditionally(EqualTo(Remainder(a,
Literal(3L)), Literal(0L))))
+ // Narrowing cast (Long -> Int), parsing cast (String -> Int), and
non-numeric cast.
Review Comment:
Fixed in 7afd4fde9bc: added
`assert(!ExprUtils.canEvaluateUnconditionally(Cast(Add(i, Literal(1)),
LongType)))` to the negative test. Verified the assertion fails if the
`canEvaluateUnconditionallyInternal(child)` conjunct is dropped from the Cast
arm while it passes with the arm as committed.
--
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]
