viirya commented on code in PR #6035:
URL: https://github.com/apache/datafusion-comet/pull/6035#discussion_r4288566394
##########
spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala:
##########
@@ -65,6 +66,70 @@ class CometNativeCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
import testImplicits._
+ test("literal cast failure in an unvisited conditional branch does not fail
planning") {
+ withTempPath { path =>
+ withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
+ spark
+ .range(2)
+ .selectExpr("id", "CAST(id AS STRING) AS value")
+ .coalesce(1)
+ .write
+ .parquet(path.getCanonicalPath)
+ }
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
+ withParquetTable(path.getCanonicalPath, "cast_branch_rows") {
+ val cast = "CAST(IF(id = 1, 'bad', value) AS INT)"
+ val masked = s"SELECT $cast AS parsed FROM cast_branch_rows LIMIT 1"
+ withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
+ assert(sql(masked).collect().toSeq == Seq(Row(0)))
+ }
+ // ConstantFolding must leave the throwing literal cast inside the
conditional.
+ val badCasts = sql(masked).queryExecution.optimizedPlan
+ .flatMap(_.expressions)
+ .flatMap(_.collect {
+ case c: Cast
+ if c.child.isInstanceOf[Literal] &&
+
Option(c.child.asInstanceOf[Literal].value).exists(_.toString == "bad") =>
+ c
+ })
+ assert(badCasts.nonEmpty)
+ val (_, plan) =
+ checkSparkAnswerAndFallbackReason(masked,
CometCast.literalCastConditionalEvalReason)
+ assert(collect(plan) { case scan: CometNativeScanExec => scan
}.nonEmpty)
+ checkSparkError(
+ sql(s"SELECT $cast FROM cast_branch_rows"),
+ "CAST_INVALID_INPUT",
+ checkNative = false)
+ }
+ }
+ }
+ }
+
+ test("successful literal casts and non-ANSI invalid casts retain native
projection") {
+ withParquetTable(Seq(Tuple1(0), Tuple1(1)), "cast_branch_controls") {
+ for (ansi <- Seq("true", "false")) {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansi) {
+ val query = "SELECT CAST(IF(_1 = 1, '1', CAST(_1 AS STRING)) AS INT)
" +
+ "FROM cast_branch_controls"
+ val (_, plan) = checkSparkAnswerAndOperator(sql(query))
+ assert(collect(plan) { case project: CometProjectExec => project
}.nonEmpty)
+ assert(
+ !plan.exists(_.getTagValue(CometExplainInfo.FALLBACK_REASONS)
+ .exists(_.contains(CometCast.literalCastConditionalEvalReason))))
Review Comment:
This assertion cannot fail, so it is not testing what it looks like it is
testing:
```scala
assert(
!plan.exists(_.getTagValue(CometExplainInfo.FALLBACK_REASONS)
.exists(_.contains(CometCast.literalCastConditionalEvalReason))))
```
`withFallbackReason(cast, ...)` tags the `Cast` **expression**, not a plan
node. An expression-level reason only reaches the operator through
`CometExecRule.rollUpFallbackReasons`, which runs on just one path:
```scala
val converted = tryConvertToComet(op, handler)
if (converted.isEmpty) {
rollUpFallbackReasons(op)
reportUnexplainedFallback(op)
}
```
In this control the conversion succeeds — that is the point of the test — so
the roll-up never runs and the plan node never carries the tag. The assertion
passes whether or not the reason was recorded on the expression.
Switching to `ExtendedExplainInfo.getFallbackReasons(plan)` would not help
either; it reads the same plan-node tags and depends on the same roll-up.
It is also redundant. `checkSparkAnswerAndOperator` already asserts through
`checkCometOperators` / `findFirstNonCometOperator` that every operator in the
plan is a Comet operator, which is a stronger statement than either this
assertion or the `CometProjectExec` collect on the line above. Dropping both
lines in each of the three control blocks would leave a clearer and strictly
stronger guarantee:
```scala
val (_, plan) = checkSparkAnswerAndOperator(sql(query))
```
If you would rather assert the reason's absence directly, it has to be read
off the expression tree rather than the plan node — e.g. collecting the `Cast`
nodes out of `plan.expressions` and checking their own tags.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -452,11 +452,14 @@ abstract class CometTestBase
}
}
- /** Checks native execution and Spark exception type, error class and
SQLSTATE parity. */
+ /** Checks Spark exception parity and, by default, native execution. */
Review Comment:
Adding `checkNative` with a `true` default is the right shape here — every
existing caller keeps the stricter behavior, and only this deliberate-fallback
case opts out while retaining the exception, error-class, SQLSTATE and
cause-chain parity checks.
Worth considering a short note in the scaladoc that `checkNative = false` is
for cases where the fallback is the expected outcome, so the escape hatch does
not get reached for merely inconvenient plan assertions later.
##########
spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala:
##########
@@ -65,6 +66,70 @@ class CometNativeCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
import testImplicits._
+ test("literal cast failure in an unvisited conditional branch does not fail
planning") {
+ withTempPath { path =>
+ withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
+ spark
+ .range(2)
+ .selectExpr("id", "CAST(id AS STRING) AS value")
+ .coalesce(1)
+ .write
+ .parquet(path.getCanonicalPath)
+ }
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
+ withParquetTable(path.getCanonicalPath, "cast_branch_rows") {
+ val cast = "CAST(IF(id = 1, 'bad', value) AS INT)"
+ val masked = s"SELECT $cast AS parsed FROM cast_branch_rows LIMIT 1"
+ withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
+ assert(sql(masked).collect().toSeq == Seq(Row(0)))
+ }
+ // ConstantFolding must leave the throwing literal cast inside the
conditional.
+ val badCasts = sql(masked).queryExecution.optimizedPlan
+ .flatMap(_.expressions)
+ .flatMap(_.collect {
+ case c: Cast
+ if c.child.isInstanceOf[Literal] &&
+
Option(c.child.asInstanceOf[Literal].value).exists(_.toString == "bad") =>
+ c
+ })
+ assert(badCasts.nonEmpty)
+ val (_, plan) =
+ checkSparkAnswerAndFallbackReason(masked,
CometCast.literalCastConditionalEvalReason)
+ assert(collect(plan) { case scan: CometNativeScanExec => scan
}.nonEmpty)
+ checkSparkError(
+ sql(s"SELECT $cast FROM cast_branch_rows"),
+ "CAST_INVALID_INPUT",
+ checkNative = false)
+ }
+ }
+ }
+ }
+
+ test("successful literal casts and non-ANSI invalid casts retain native
projection") {
Review Comment:
Minor: the test name says "retain native projection", which is what the
assertions check, but the more valuable property here is that a *successful*
literal cast still takes the folding path rather than the new fallback — that
is the over-correction guard. Since the fallback-reason assertion above does
not actually establish that (see the other comment), consider either asserting
the folded literal appears in the serialized projection, or renaming to reflect
what is really covered, so a future reader does not over-trust it.
--
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]
