viirya commented on code in PR #6826: URL: https://github.com/apache/datafusion-comet/pull/6826#discussion_r4266293055
########## 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: Done in 594d7d911, after merging main to pick up `expect_error_class`. All five pass, so the errors raised through the dispatcher match Spark's exception type, error class and SQLSTATE, and they now fail if the projection falls back. ########## 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: Agreed, `convert` is back to what it is on main. `getSupportLevel` now checks `Try(cast.eval()).isFailure` and returns `Unsupported` for a failing literal, so `foldLiteralCast` and the unreachable branch are gone, and the reason string is `private`. ########## 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: Trimmed in 594d7d911. The Scala test now only covers the `'1e-2147483647'` root-cause comparison. The SQL file covers the rest with `expect_dispatch` and `expect_error_class`. ########## 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: Filed #6966 for both shapes, and linked it from here, from the early zero return above, and from the negative-scale test. -- 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]
