snmvaughan commented on code in PR #7171:
URL: https://github.com/apache/datafusion-comet/pull/7171#discussion_r4364406121
##########
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.
Review Comment:
Thanks, I checked: the SPARK-60119 back-ports are in 4.0.5-rc3, 4.1.4-rc4
and 4.2.1-rc3. When the scale change is over 38, `changePrecision` now rounds a
value below 0.01 ulp of the target scale as 0.01 ulp of the same sign, so
`CAST('1e-2147483647' AS DECIMAL(10,2))` gives `0.00`, as the native cast
already does. In 4c92600 the audit entry and the two `string.rs` comments say
that the positive-scale case of #6966 applies only before those releases.
Its negative-scale case still differs, though. With
`allowNegativeScaleOfDecimal=true`, `'1e-2147483647'` to `DECIMAL(10,-2)` now
gives 0 in Spark, while the native cast raises `CAST_INVALID_INPUT` under ANSI
and returns NULL otherwise. So `(ansi) cast string to negative-scale decimal
fails when rescaling overflows an int` will fail once Comet builds against
these releases. I'd like to handle that separately, gated on SPARK-60119 like
these two, so this PR stays on SPARK-58821 and SPARK-60118.
##########
native/spark-expr/src/comet_scalar_funcs.rs:
##########
@@ -133,6 +133,13 @@ pub fn create_comet_physical_fun_with_eval_mode(
"spark_sequence" => {
make_comet_scalar_udf!("spark_sequence", spark_sequence, data_type)
}
+ "spark_sequence_recomputed" => {
Review Comment:
Thanks for the heads-up. Both names already share one kernel,
`sequence(args, data_type, OverflowedLength)`, so the merge is small: whichever
PR lands second gives `SparkSequence` the overflow mode as a field and
constructs it for both names. If #6948 lands first I'll rebase this onto it
that way, and otherwise I'll point #6948 here.
##########
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
+ * them. The fixes leave nothing in the plan for Comet to detect, so the
checks go by the version.
Review Comment:
Added in 4c92600, on the cast and `sequence` pages rather than
`spark-versions.md`, so the note sits next to the behavior as it does in
`floating-point.md`. The cast page's "String to Decimal" section and
`CometSequence.getCompatibleNotes`, which GenerateDocs puts on the array page,
say that Comet chooses by the Spark version number. They also say what a vendor
build that reports an older version but includes the fix gets instead: NULL for
`'0e39'` (or an error under ANSI), and the internal error for the overflowing
`sequence`, where Spark returns values.
##########
native/spark-expr/src/array_funcs/sequence.rs:
##########
@@ -425,6 +474,59 @@ mod tests {
assert!(err.contains("Unreachable code reached."), "{err}");
}
+ /// SPARK-58821's cases: `stop - start` overflows a long but a large step
keeps the exact
+ /// length small, so `spark_sequence_recomputed` returns the elements
instead of the internal
+ /// error. Overflows whose length really is too large still raise the
limit error.
+ #[test]
+ fn recomputed_length_after_overflow() {
+ let args = vec![
+ ColumnarValue::Array(Arc::new(Int64Array::from(vec![
+ Some(i64::MIN),
+ Some(i64::MIN),
+ Some(i64::MAX),
+ ]))),
+ ColumnarValue::Array(Arc::new(Int64Array::from(vec![
+ Some(i64::MAX),
+ Some(0),
+ Some(i64::MIN),
+ ]))),
+ ColumnarValue::Array(Arc::new(Int64Array::from(vec![
+ Some(i64::MAX),
+ Some(4611686018427387904),
+ Some(-i64::MAX),
+ ]))),
+ ];
+ let ColumnarValue::Array(arr) =
+ spark_sequence_recomputed(&args,
&list_of(DataType::Int64)).unwrap()
+ else {
+ unreachable!("array inputs produce an array")
+ };
+ let list = as_primitive_list(&arr);
+ assert_eq!(row_values(&list, 0), vec![i64::MIN, -1, i64::MAX - 1]);
+ assert_eq!(
+ row_values(&list, 1),
+ vec![i64::MIN, -4611686018427387904, 0]
+ );
+ assert_eq!(row_values(&list, 2), vec![i64::MAX, 0, -i64::MAX]);
+
+ // The released behaviour, on the same rows, is the internal error.
Review Comment:
Done in 4c92600. `recomputed_length_after_overflow` keeps only the element
checks. A new `SparkFixesSuite` holds one version table for both fixes and
`CometSequence.nativeFunction`, and the two suites keep only their `convert`
checks, which depend on the running Spark version. The new suite is in both PR
build workflows.
--
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]
