comphead commented on code in PR #6826:
URL: https://github.com/apache/datafusion-comet/pull/6826#discussion_r4265558466
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -121,7 +130,14 @@ object CometCast
val cometEvalMode = evalMode(cast)
cast.child match {
case _: Literal =>
- exprToProtoInternal(Literal.create(cast.eval(), cast.dataType),
inputs, binding)
+ foldLiteralCast(cast) match {
+ case Some(folded) => exprToProtoInternal(folded, inputs, binding)
+ case None =>
+ // `getSupportLevel` reports a failing literal cast as
`Unsupported`, so it never
+ // reaches here through `exprToProtoInternal`.
+ withFallbackReason(cast, failedLiteralCastReason)
+ None
+ }
case _ =>
Review Comment:
The `None` branch here cannot be reached. `convert` is only called from
`exprToProtoInternal` after `getSupportLevel`, and `getSupportLevel` now
returns `Compatible()` only when `foldLiteralCast` succeeded, otherwise
`Unsupported` goes to the dispatcher. I could not find a caller of
`CometCast.convert` outside that path. Could `convert` stay as it is on `main`
(`Literal.create(cast.eval(), cast.dataType)`)? Then `getSupportLevel` can use
`Try(cast.eval()).isSuccess` (NonFatal only, same as now), and
`foldLiteralCast`, its `case _ => None` branch for a non-literal child that no
caller can pass, and the second use of `failedLiteralCastReason` all go away.
The constant is also `private[comet]` but is not referenced outside this file,
so `private` would be enough if it stays.
##########
spark/src/test/resources/sql-tests/expressions/cast/cast_literal_ansi.sql:
##########
@@ -0,0 +1,94 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- A cast of a literal that fails under ANSI must not fail the query while
Comet plans it. Spark
+-- raises the error only when a row reaches the cast, so a branch no row
chooses returns rows.
+-- ConstantFolding is left enabled: it keeps a failing cast unfolded inside a
conditional branch,
+-- which is how such a cast reaches Comet in a real query. Comet runs such a
cast with Spark's
+-- own Cast through the JVM codegen dispatcher.
+
+-- Config: spark.sql.ansi.enabled=true
+-- ConstantFolding: enabled
+
+statement
+CREATE TABLE test_cast_literal_ansi(id bigint) USING parquet
+
+statement
+INSERT INTO test_cast_literal_ansi VALUES (0), (1), (2)
+
+-- no row reaches the failing cast
+query expect_dispatch(cast)
+SELECT id, CASE WHEN id = 5 THEN CAST('bad' AS BIGINT) ELSE id END FROM
test_cast_literal_ansi
+
+query expect_dispatch(cast)
+SELECT id, IF(id > 5, CAST('2147483648' AS INT), 0) FROM test_cast_literal_ansi
+
+query expect_dispatch(cast)
+SELECT id, COALESCE(id, CAST('bad' AS BIGINT)) FROM test_cast_literal_ansi
+
+query expect_dispatch(cast)
+SELECT id, CASE WHEN id = 5 THEN CAST('NaNF' AS DOUBLE) ELSE 0D END,
+ IF(id > 5, CAST('InfinityD' AS FLOAT), 0F) FROM test_cast_literal_ansi
+
+query expect_dispatch(cast)
+SELECT id, CASE WHEN id = 5 THEN CAST('1e-2147483648' AS DECIMAL(10,2))
+ ELSE CAST(0 AS DECIMAL(10,2)) END FROM test_cast_literal_ansi
+
+-- a cast of a literal that does not fail is still folded
+query
+SELECT id, CASE WHEN id = 1 THEN CAST('7' AS BIGINT) ELSE id END FROM
test_cast_literal_ansi
+
+-- a row reaches the failing cast
+query expect_error(CAST_INVALID_INPUT)
Review Comment:
These five `expect_error` queries only check that Spark and Comet both fail
with a message containing the class. They do not check Comet operators, so if
the cast stops reaching the dispatcher and the projection falls back to Spark,
they still pass because Spark raises the same error. The `expect_dispatch`
queries above only guard the unreached variants. Could you switch the queries
at lines 56, 59, 62, 65 (`CAST_INVALID_INPUT`) and 69 (`CAST_OVERFLOW`) to
`expect_error_class(...)`? That checks the operators and also the exception
type, error class and SQLSTATE against Spark, which is what the Scala test does
today with `checkSparkError`.
##########
spark/src/test/scala/org/apache/comet/CometNativeCastSuite.scala:
##########
@@ -1416,6 +1416,62 @@ class CometNativeCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
}
}
+ test("(ansi) failing cast of a literal in an unreached branch does not fail
planning") {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
+ withTable("cast_literal_branch") {
+ Seq(0L, 1L,
2L).toDF("id").write.format("parquet").saveAsTable("cast_literal_branch")
+ // ConstantFolding leaves a failing cast unfolded inside a conditional
branch, so it
+ // reaches Comet as a cast of a literal. No row chooses the branch, so
Spark returns rows.
+ // Comet runs Spark's own Cast through the codegen dispatcher, not a
native cast.
+ checkSparkAnswerAndImpl(
+ sql("""SELECT id,
+ | CASE WHEN id = 5 THEN CAST('bad' AS BIGINT) ELSE id END,
+ | IF(id > 5, CAST(2147483648L AS INT), 0)
+ |FROM cast_literal_branch""".stripMargin),
+ dispatched = Seq("cast"))
+ // A row that chooses the branch raises Spark's error.
+ checkSparkError(
+ sql(
+ "SELECT CASE WHEN id = 1 THEN CAST('bad' AS BIGINT) ELSE id END " +
+ "FROM cast_literal_branch"),
+ "CAST_INVALID_INPUT")
+ checkSparkError(
+ sql("SELECT IF(id > 1, CAST(2147483648L AS INT), 0) FROM
cast_literal_branch"),
+ "CAST_OVERFLOW")
Review Comment:
This block repeats `cast_literal_ansi.sql`. The unreached `CASE`/`IF` case,
the `CAST_INVALID_INPUT` case and the `CAST_OVERFLOW` case are all in the SQL
file with the same queries, and once those use `expect_error_class` the SQL
file checks the same things (dispatch, operators, class, SQLSTATE). Could this
test keep only what the SQL framework cannot express, which is the
`'1e-2147483647'` root-cause comparison (a raw `ArithmeticException` with no
error class, so it needs the Scala helpers)? The negative-scale test below is
fine as a Scala test because `DECIMAL(10,-2)` does not parse in SQL.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -629,10 +650,10 @@ fn parse_string_to_decimal(input_str: &str, precision:
u8, scale: i8) -> SparkRe
mantissa.checked_mul(pow10_i128(scale_adjustment as u32).unwrap())
} else {
// Need to divide (decrease scale)
- let abs_scale_adjustment = (-scale_adjustment) as u32;
- if abs_scale_adjustment > 38 {
+ if -scale_adjustment > 38 {
Review Comment:
Could we have a tracking issue for the remaining string to decimal gap,
linked from here and from the Scala test? Two shapes I can see from reading the
code (the Spark side is taken from the thread, not run by me). First, a column
`'1e-2147483647'` cast to `DECIMAL(10,2)` hits this early `Ok(Some(0))` and
returns `0.00`, while Spark raises `ArithmeticException` in every eval mode per
your comment. That predates this PR. Second, the new `i32::try_from` guard
returns `Err`, which `cast_string_to_decimal128_impl` turns into NULL outside
ANSI and into `CAST_INVALID_INPUT` under ANSI, so the negative-scale test only
pins that both engines fail and not Spark's exception type or its non-ANSI
result.
--
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]
