snmvaughan commented on code in PR #7171:
URL: https://github.com/apache/datafusion-comet/pull/7171#discussion_r4378333531
##########
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:
Done in 1755de3: the else branch is now `checkSparkError(sql(query),
"INTERNAL_ERROR")`. Spark 3.4 and 3.5 raise the same
`internalError("Unreachable code reached.")`, and their shims map it to
`SparkException.internalError`, so it should hold on those profiles too.
##########
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:
Thanks, added in 1755de3. The entry now says that SPARK-60118 counts the
digits in a long, what released Spark does with those three strings (and with
`'0e2147483647'`), and that the native cast matches the fixed releases outside
ANSI and raises `CAST_INVALID_INPUT` under it, with a link to #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:
Good catch. In 1755de3 the released path counts a zero's integral digits as
`1 - scale` with a wrapping `i32` subtraction, as Spark's int arithmetic does,
so `'0e2147483647'` casts to 0 there and no other input changes.
`test_parse_string_to_decimal_zero_with_large_exponent` now passes
`'0e2147483647'` in both modes, and still rejects `'0e2147483646'` and
`'0.0e2147483647'`, whose counts don't wrap, on the released path.
##########
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:
Thanks for running both suites with the RC classes. I'll handle `(ansi)
failing cast of a literal raises Spark's own exception when reached` together
with the negative-scale test in the SPARK-60119 change, and I've noted it on
#7187.
##########
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:
I've taken the scaladoc route here: in 1755de3 it names the two fixes it
covers and points to `RegrSparkVersions` and `ArraySetSupport.normalizesFloats`
for the others. Moving those in is worth doing for the reason you give. I'd do
it in a separate PR, since the two read versions a little differently:
`RegrSparkVersions` accepts any string that starts with `x.y` and falls back to
a string comparison, while `SparkFixes` uses Spark's `VersionUtils` and treats
a version it can't parse as unfixed. Folding them together means picking one
rule, so I've opened #7241 for it.
--
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]
