sunchao commented on code in PR #6826:
URL: https://github.com/apache/datafusion-comet/pull/6826#discussion_r4236061109
##########
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:
[P2] [P2] Preserve decimal failures even when the scale fits in i32. With
ANSI enabled and a Parquet table `t(id BIGINT)` containing `(0), (1)`, run
`SELECT CASE WHEN id = 1 THEN CAST('1e-2147483647' AS DECIMAL(10,2)) ELSE
CAST(0 AS DECIMAL(10,2)) END FROM t`. On the tested JDK 21 runtime, Spark
retains this cast and throws `ArithmeticException: BigInteger would overflow
supported range` when the branch is selected. This failed-fold route instead
admits the native decimal cast, whose parser returns `0.00`. The base raised
the Spark exception through `cast.eval()`, so successful literal evaluation is
newly exposed by this PR. This case has an in-range scale and survives the new
overflow guard. Please retain Spark execution for such failed folds or
reproduce its runtime failure, and correct the new Rust assertion that
explicitly expects zero for this input.
Evidence: Bounded Catalyst probes on Spark 3.5.9, 4.1.3 and 4.2.0 with
OpenJDK 21.0.12.1 printed `foldLiteralCast=None retainedCasts=1`, returned
`0.00` for id=0, and threw `BigInteger would overflow supported range` for
id=1. The exact-head extracted Rust parser, built in release mode, returned
`Ok(Some(0))`. Source tracing confirms `canCastFromString` marks DecimalType
compatible, scalar `spark_cast` reaches `cast_string_to_decimal128_impl`, and
`Ok(Some(0))` is accepted in ANSI mode. No end-to-end Comet query was run.
##########
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:
[P2] [P2] Use a JDK-independent invalid exponent in the result-comparison
fixture. The added `'1e2147483648'` row makes the final plain `query` fail on
OpenJDK 21.0.12.1: Spark's `try_cast(s AS DECIMAL(10,2))` throws
`ArithmeticException: Underflow` instead of returning NULL. This breaks the new
fixture on a supported JDK 21 configuration even though default JDK 17 CI can
pass. This JDK accepts the string's BigDecimal scale of `Integer.MIN_VALUE`,
then fails during Spark's decimal conversion. Please replace this sample with
an unambiguously invalid exponent such as `'1e2147483649'` for the NULL-result
comparison, or isolate it with the correct JDK-specific exceptional behavior.
Evidence: Executed the final SELECT against a Parquet string column on Spark
3.5.9 and an RDD-backed string column on Spark 4.1.3, both using OpenJDK
21.0.12.1. Both generated execution plans contained `try_cast`, and collection
failed with root cause `java.lang.ArithmeticException: Underflow`. Catalyst
probes reproduced it on Spark 4.2.0 as well. OpenJDK source comparison confirms
JDK 17 rejects the exponent before decimal construction, explaining why its
behavior differs.
--
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]
