viirya commented on code in PR #6826:
URL: https://github.com/apache/datafusion-comet/pull/6826#discussion_r4236499356
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -119,10 +121,10 @@ object CometCast
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
val cometEvalMode = evalMode(cast)
- cast.child match {
- case _: Literal =>
- exprToProtoInternal(Literal.create(cast.eval(), cast.dataType),
inputs, binding)
- case _ =>
+ foldLiteralCast(cast) match {
+ case Some(folded) =>
+ exprToProtoInternal(folded, inputs, binding)
+ case None =>
Review Comment:
Agreed, chasing parser parity one input at a time is the wrong approach. A
failed fold already proves that Spark's Cast throws for this literal, so
17efb270c reports it as `Unsupported` instead of trying the native cast.
`CometCast` is a `CodegenDispatchFallback`, so the cast runs Spark's own `Cast`
through the JVM codegen dispatcher and keeps the operator native, or the
projection falls back to Spark when the dispatcher is disabled. Either way the
error, including the raw `ArithmeticException` for `'1e-2147483647'`, comes
from Spark and is raised only when a row reaches the cast. The SQL fixture now
asserts `expect_dispatch(cast)` for the unreached branches, and the Scala test
checks that Comet and Spark fail with the same root-cause exception for
`'1e-2147483647'`. I removed the Rust assertion that it parses to zero.
The two earlier parser fixes stay, since they fix column inputs (e.g.
`try_cast(s AS DOUBLE)` with `s = 'NaNF'`), but this path no longer depends on
them.
##########
spark/src/test/resources/sql-tests/expressions/cast/cast_literal_ansi.sql:
##########
@@ -0,0 +1,93 @@
+-- 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.
+
+-- 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
+SELECT id, CASE WHEN id = 5 THEN CAST('bad' AS BIGINT) ELSE id END FROM
test_cast_literal_ansi
+
+query
+SELECT id, IF(id > 5, CAST('2147483648' AS INT), 0) FROM test_cast_literal_ansi
+
+query
+SELECT id, COALESCE(id, CAST('bad' AS INT)) FROM test_cast_literal_ansi
+
+-- Java only allows a D/F suffix after a decimal number, so these special
values are invalid
+query
+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
+
+-- java.math.BigDecimal rejects a scale outside the int range
+query
+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 accepted
+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)
+SELECT id, CASE WHEN id = 1 THEN CAST('bad' AS BIGINT) ELSE id END FROM
test_cast_literal_ansi
+
+query expect_error(CAST_INVALID_INPUT)
+SELECT id, CASE WHEN id = 1 THEN CAST('NaNF' AS DOUBLE) ELSE 0D END FROM
test_cast_literal_ansi
+
+query expect_error(CAST_INVALID_INPUT)
+SELECT id, IF(id > 1, CAST('InfinityD' AS FLOAT), 0F) FROM
test_cast_literal_ansi
+
+query expect_error(CAST_INVALID_INPUT)
+SELECT id, CASE WHEN id = 1 THEN CAST('1e-2147483648' AS DECIMAL(10,2))
+ ELSE CAST(0 AS DECIMAL(10,2)) END FROM test_cast_literal_ansi
+
+query expect_error(CAST_OVERFLOW)
+SELECT id, IF(id > 1, CAST(2147483648L AS INT), 0) FROM test_cast_literal_ansi
+
+-- The same floating-point parsing applies to a column. Only the exact-case
signed NaN is valid.
+statement
+CREATE TABLE test_cast_float_special(s string) USING parquet
+
+statement
+INSERT INTO test_cast_float_special VALUES ('NaNF'), ('NaND'), ('nanf'),
('-nan'), ('+nan'),
+ ('-NaN'), ('+NaN'), ('NaN'), ('InfinityD'), ('infF'), ('-InfinityF'),
('Infinity'), ('-inf'),
+ ('1.5d'), ('-.5F'), ('1e5d'), ('d'), ('1.0dd'), (NULL)
+
+query
+SELECT s, try_cast(s AS DOUBLE), try_cast(s AS FLOAT) FROM
test_cast_float_special
+
+statement
+CREATE TABLE test_cast_decimal_scale(s string) USING parquet
+
+statement
+INSERT INTO test_cast_decimal_scale VALUES ('1e-2147483648'),
('1.0e-2147483647'),
+ ('0e-2147483648'), ('1e2147483648'), ('1e-9999999999'), ('1.5e-3'),
('12.345e1'), (NULL)
Review Comment:
Thanks, replaced it with `'1e2147483649'`, which is invalid on every JDK, in
both the SQL fixture and the Rust test (17efb270c).
--
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]
