andygrove commented on code in PR #7171:
URL: https://github.com/apache/datafusion-comet/pull/7171#discussion_r4371020889
##########
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##########
@@ -1776,4 +1776,41 @@ class CometArrayExpressionSuite extends CometTestBase
with AdaptiveSparkPlanHelp
}
}
}
+
+ // SparkFixesSuite covers the variant for each Spark version.
+ test("sequence asks for the native variant of the Spark version it runs on")
{
+ val start = AttributeReference("start", LongType)()
+ val stop = AttributeReference("stop", LongType)()
+ val step = AttributeReference("step", LongType)()
+ val proto = CometSequence.convert(
+ Sequence(start, stop, Some(step)),
+ Seq(start, stop, step),
+ binding = false)
+ assert(
+ proto.map(_.getScalarFunc.getFunc) ==
+ Some(CometSequence.nativeFunction(org.apache.spark.SPARK_VERSION)))
+ }
+
+ // When stop - start overflows a long but the sequence is short, Spark
returns its elements
+ // where releases without SPARK-58821 raise INTERNAL_ERROR("Unreachable code
reached.").
+ test("sequence whose stop - start overflows a long") {
+ val rows = Seq(
+ (Long.MinValue, Long.MaxValue, Long.MaxValue),
+ (Long.MinValue, 0L, 1L << 62),
+ (Long.MaxValue, Long.MinValue, -Long.MaxValue))
+ withParquetTable(rows, "t") {
+ val query = "SELECT _1, _2, _3, sequence(_1, _2, _3) FROM t"
+ if
(SparkFixes.sequenceRecomputesOverflowedLength(org.apache.spark.SPARK_VERSION))
{
+ checkSparkAnswerAndOperator(query)
+ } else {
+ val df = sql(query)
Review Comment:
Nit, optional: the else branch only checks that both messages contain
"Unreachable code reached", so it would still pass if the native error came
back as a `CometNativeException` with the same text instead of Spark's
`INTERNAL_ERROR`. Could it be `checkSparkError(sql(query), "INTERNAL_ERROR")`?
That checks the plan is native and that the exception class, error class and
SQLSTATE match Spark's. I tried that form on 4.1.3 and it passes.
##########
spark/src/main/scala/org/apache/comet/serde/SparkFixes.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.
+ */
+
+package org.apache.comet.serde
+
+import org.apache.spark.sql.comet.util.Utils
+
+/**
+ * Spark fixes that change a result Comet reproduces natively, and the Spark
releases that carry
Review Comment:
Nit, optional: the scaladoc reads like the one place for Spark fixes that
Comet picks by version. But `RegrSparkVersions` (SPARK-48719, SPARK-55969) and
`ArraySetSupport.normalizesFloats` (SPARK-54918, SPARK-59602) keep their own
version tables, and `RegrSparkVersions` has its own version parser. Could
`SparkFixes` take those as well, with their tables moving into
`SparkFixesSuite`? A fork that backports one of these fixes would then have one
file to change. If that's more than this PR should take on, a scaladoc that
says it covers these two would work too.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -725,8 +741,9 @@ 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)
- // Spark's `BigDecimal.setScale` also fails for a very large
difference that still fits in
- // an int, while this rounds to zero:
https://github.com/apache/datafusion-comet/issues/6966
+ // Before SPARK-60119 (Spark 4.0.5, 4.1.4 and 4.2.1), Spark's
`BigDecimal.setScale` also
Review Comment:
Nit, optional: a heads-up for the Spark bump. With the 4.1.4-rc4
`Decimal.scala` on the test classpath, `(ansi) failing cast of a literal raises
Spark's own exception when reached` in `CometNativeCastSuite` fails with "Spark
did not fail", because SPARK-60119 makes `CAST('1e-2147483647' AS
DECIMAL(10,2))` return `0.00`. #7187 mentions the negative-scale test but not
this one, so the SPARK-60119 change will need to handle both. Across all of
`CometNativeCastSuite` and `CometSqlFileTestSuite`, those two were the only
failures I could put down to these releases.
##########
docs/source/contributor-guide/expression-audits/conversion_funcs.md:
##########
@@ -27,6 +27,7 @@
- Spark 3.5.8 (audited 2026-05-27): baseline. `Cast(child, dataType,
timeZoneId, evalMode)`; eval modes are `LEGACY`, `ANSI`, `TRY`. The legacy
`Cast.canCast` matrix and the `Cast.canAnsiCast` matrix decide acceptance per
type pair. Comet routes via `CometCast`
(`spark/src/main/scala/org/apache/comet/expressions/CometCast.scala`) using a
per-source-type support matrix that returns `Compatible`,
`Incompatible(reason)`, or `Unsupported(reason)`; literal children are
short-circuited to `Compatible()` so `CometLiteral` validates them. The
serialized `Cast` proto carries `datatype`, `evalMode`, `timezone` (default
`UTC`), and `isSpark4Plus`; `allowIncompatible` is enforced only by the JVM
planner and is not sent to the native cast kernel. The native side
(`native/spark-expr/src/conversion_funcs/cast.rs`) implements explicit
per-eval-mode branches for narrowing numeric casts that match Spark's overflow
exceptions, and falls through to DataFusion `cast_with_options(safe = !ANSI)`
for the r
est.
- Spark 4.0.1 (audited 2026-05-27): `VariantType` added; `StringType` literals
replaced with `_: StringType` to accommodate collated strings. `(TimestampType,
ByteType|ShortType|IntegerType)` added to `canAnsiCast`. `NullIntolerant` ->
`nullIntolerant: Boolean` refactor. New `ToPrettyString.BinaryFormatter`
semantics for `Binary -> String` are replicated natively via
`spark_binary_formatter`. Numeric-to-numeric matrix unchanged.
- Spark 4.1.1 (audited 2026-05-27): `TimeType` added; many `TimeType` arms in
`canCast`/`canAnsiCast`. Geospatial `GeographyType` / `GeometryType` types
added with their own conversion rules. Numeric-to-numeric matrix unchanged.
+- Spark 4.0.5, 4.1.4 and 4.2.1 (release candidates; in 4.0.5-rc3, 4.1.4-rc4
and 4.2.1-rc3): `Decimal.fromString` and `fromStringANSI` no longer count
integral digits for zero (SPARK-60118), so a zero with a large exponent such as
`'0e39'` casts to 0 instead of NULL (or `NUMERIC_OUT_OF_SUPPORTED_RANGE` under
ANSI). `CometCast` sets the `decimal_zero_any_exponent` cast option from
`SparkFixes.castsZeroWithAnyExponentToDecimal`, and `parse_string_to_decimal`
skips the zero limit when it is set. The same releases skip
`BigDecimal.setScale` in `Decimal.changePrecision` when the value's magnitude
already decides the result (SPARK-60119), so a string far below the target
scale's ulp casts to the rounded value instead of throwing
`ArithmeticException`. `'1e-2147483647'` to `DECIMAL(10,2)` gives `0.00`, which
the native cast already returns, so that case of
[#6966](https://github.com/apache/datafusion-comet/issues/6966) applies only to
earlier releases. Its negative-scale case (`spark.sql.le
gacy.allowNegativeScaleOfDecimal=true`) still differs: `'1e-2147483647'` to
`DECIMAL(10,-2)` gives 0 on these releases, while the native cast raises
`CAST_INVALID_INPUT` under ANSI and returns NULL otherwise.
Review Comment:
Nit, optional: SPARK-60118 also fixes the overflow in the integral-digit
check itself. It now counts the digits in a long, so `'1e2147483647'`,
`'12e2147483647'` and `'1.5e2147483647'` return NULL, or raise
`NUMERIC_OUT_OF_SUPPORTED_RANGE` under ANSI. Released Spark throws a raw
`ArithmeticException` there in every mode. I ran these on 4.1.3, and on 4.1.3
with the 4.1.4-rc4 `Decimal.scala` on the test classpath and `SPARK_VERSION`
reporting 4.1.4. Comet returns NULL in legacy and try mode on both, so it
matches the new releases there. Under ANSI it raises `CAST_INVALID_INPUT`,
which is #7188. Could this entry describe that half too and link #7188?
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -695,9 +708,11 @@ fn parse_string_to_decimal(input_str: &str, precision: u8,
scale: i8) -> SparkRe
// validate and parse mantissa and exponent or bubble up the error
let (mantissa, exponent) = parse_decimal_str(trimmed, input_str,
precision, scale)?;
- // Early return mantissa 0, Spark checks if it fits digits and throw error
in ansi
+ // Early return mantissa 0, Spark checks if it fits digits and throw error
in ansi. Spark
+ // counts precision - scale integral digits for zero too, so "0e38" (scale
-38) has 39 and is
+ // over the 38-digit limit. Spark stopped applying that limit to zero in
SPARK-60118.
if mantissa == 0 {
- if exponent < -37 {
+ if !zero_any_exponent && exponent < -37 {
Review Comment:
Nit, optional: the same overflow lets one zero through on released Spark.
For `'0e2147483647'` the digit count is 1 - (-2147483647), which wraps negative
in Spark's int arithmetic and skips the limit. So 4.1.3 returns `0.00`, while
this branch returns NULL, or `NUMERIC_OUT_OF_SUPPORTED_RANGE` under ANSI. Since
the `zero_any_exponent = false` path now stands for the released rule, would it
be worth letting an exponent of exactly 2147483647 through here, and adding it
to `test_parse_string_to_decimal_zero_with_large_exponent`?
--
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]
