comphead commented on code in PR #7171:
URL: https://github.com/apache/datafusion-comet/pull/7171#discussion_r4361443891
##########
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:
The same release candidates also carry SPARK-60119, which makes
`Decimal.changePrecision` skip `setScale` when the result is known from the
magnitude (the local Spark clone has the backports `b298ec9f25b`, `a311ee20606`
and `eaed2f091c8`). From my reading of its commit message, a string far below
the target scale such as `CAST('1e-2147483647' AS DECIMAL(10,2))` then returns
`0.00` instead of throwing. Comet already rounds those to zero in `string.rs`,
so on 4.0.5, 4.1.4 and 4.2.1 it would match Spark there, and the #6966 comments
in `string.rs` then describe only the older releases. Could this entry mention
SPARK-60119 and say that #6966 applies only before those releases?
##########
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:
Heads-up on #6948, which is open and not a draft. It changes the
`spark_sequence` arm to construct `SparkSequence` with the session memory pool
so the kernel reserves memory, and it leaves this arm alone. Whichever PR lands
second needs `spark_sequence_recomputed` to go through the same
`SparkSequence`, for example with the overflow mode as a field. Otherwise the
recomputed variant, which is the one 4.0.5 and later will use, skips the
reservation. I read this from the #6948 diff and did not build the merge.
##########
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:
Two blocks in `recomputed_length_after_overflow` repeat existing coverage.
The `spark_sequence` internal error on `(i64::MIN, i64::MAX, i64::MAX)` is
already asserted in `length_limit_and_overflow_edges`, and the `(0, i64::MAX,
1)` limit error is asserted there for `spark_sequence` and now by the grid in
`checked_length_matches_exact_released_spark_arithmetic` in both modes. Keeping
only the element checks would make this test about what is new. In the Scala
suites, `sequence native variant by Spark version` and `cast StringType to
DecimalType zero with any exponent by Spark version` pin the same shared
`fixedIn` with near-identical version lists, so one table would do.
##########
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:
The version-number caveat for the precedent this follows is written down for
users: `compatibility/floating-point.md` says a vendor build that reports an
older version but carries SPARK-59602 behaves differently. I could not find a
user-guide mention of SPARK-58821 or SPARK-60118 (`git grep` matches only the
contributor audits and code). On a build that reports 4.0.4 but carries these
backports, Comet would return NULL for `'0e39'` and raise an internal error for
the overflowing `sequence` where Spark returns values. Could
`spark-versions.md`, or the cast and `sequence` compatibility pages, carry the
same note?
--
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]
