Skip to content

fix: [branch-1.1] match SPARK-58821 and SPARK-60118 in sequence and string to decimal cast (#7171) - #7266

Merged
andygrove merged 1 commit into
apache:branch-1.1from
snmvaughan:backport-7171-branch-1.1
Oct 27, 2026
Merged

andygrove merged 1 commit into
apache:branch-1.1from
snmvaughan:backport-7171-branch-1.1

Conversation

@snmvaughan

@snmvaughan snmvaughan commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Backport of #7171 (squash commit 1dec4c0) to branch-1.1, for #7170.

Rationale for this change

Spark 4.0.5, 4.1.4 and 4.2.1, now in release candidates, change two results that Comet reproduces natively:

  • When stop - start overflows a long but the sequence is short, sequence returns its elements, where earlier releases raise INTERNAL_ERROR (SPARK-58821).
  • A string zero with a large exponent, such as '0e39', casts to 0 instead of NULL, or NUMERIC_OUT_OF_SUPPORTED_RANGE under ANSI (SPARK-60118).

Without this change, Comet 1.1 on those releases raises the internal error and returns NULL where Spark doesn't. See #7170 and #7171.

What changes are included in this PR?

git cherry-pick -x of 1dec4c0, adapted because branch-1.1 predates parts of main next to #7171's lines. The behavior matches main:

  • sequence.rs: branch-1.1's kernel takes the start, stop and step arrays and a conversion closure, and its sequence_length flags the overflow itself, so the OverflowedLength choice is threaded through that code. recomputed_length_after_overflow also checks that a length over the limit is still an error with spark_sequence_recomputed, which main's length grid covers.
  • string.rs: the zero_any_exponent change and its test. The SPARK-60119 comment rewording is dropped, since branch-1.1 doesn't have the scale-adjustment comments it edits.
  • planner.rs: branch-1.1's SparkCastOptions::new_with_version still takes allow_incompat.
  • The contributor-guide audit entries are dropped, since that guide isn't published from release branches and they describe main's code. SparkFixes' scaladoc doesn't mention ArraySetSupport.normalizesFloats, which branch-1.1 doesn't have.
  • Import lists keep branch-1.1's names plus the ones fix: match SPARK-58821 and SPARK-60118 in sequence and string to decimal cast #7171 adds.

Backports to other branches: branch-1.2, #7263.

How are these changes tested?

The tests come with the cherry-pick, plus the extra length-limit check above. On this branch, all 978 datafusion-comet-spark-expr unit tests pass, cargo fmt --check, cargo clippy --all-targets --workspace -- -D warnings and spotless are clean, and the Scala code compiles, with SparkFixesSuite passing. I haven't run the other Scala suites locally.

…mal cast (apache#7171)

* fix: match SPARK-58821 and SPARK-60118 in sequence and string to decimal cast

Spark 4.0.5, 4.1.4 and 4.2.1, now in release candidates (4.0.5-rc3,
4.1.4-rc4 and 4.2.1-rc3), change two results that Comet reproduces
natively:

- SPARK-58821: when stop - start overflows a long but the length is small,
  sequence() returns the elements, where earlier releases raise
  INTERNAL_ERROR("Unreachable code reached."). A new native variant,
  spark_sequence_recomputed, does the same, and
  CometSequence.nativeFunction picks it on those releases.
- SPARK-60118: casting a string to decimal no longer applies the
  integral-digit limit to zero, so a zero with a large exponent ("0e79")
  casts to 0 instead of NULL (or NUMERIC_OUT_OF_SUPPORTED_RANGE under ANSI).
  CometCast sets a new Cast proto field, decimal_zero_any_exponent, and
  parse_string_to_decimal skips the zero limit when it is set.

SparkFixes holds the version checks. The internal-error case in
sequence.sql moves to CometArrayExpressionSuite, because SQL file version
directives compare only major.minor.

* docs: describe SPARK-60119 and the version-number caveat, and share one version table

Review follow-up.

- SPARK-60119, which is in the same release candidates, makes Spark return
  0.00 for a string far below the target scale's ulp, as the native cast
  already does. The cast audit entry and the string.rs comments now say
  that apache#6966's positive-scale case applies only before 4.0.5, 4.1.4 and
  4.2.1, and that its negative-scale case still differs on them.
- The cast compatibility page and CometSequence's compatibility notes say
  that Comet chooses by the Spark version number, and what a vendor build
  that reports an older version but includes SPARK-60118 or SPARK-58821
  returns, as floating-point.md does for SPARK-59602.
- recomputed_length_after_overflow keeps only the new element checks.
  length_limit_and_overflow_edges and the length grid already cover the
  internal error and both variants' limit errors.
- SparkFixesSuite holds one version table for both fixes and
  CometSequence.nativeFunction, replacing the near-identical tables in
  CometArrayExpressionSuite and CometNativeCastSuite, which keep their
  convert checks. It is listed in both PR build workflows.

* fix: follow released Spark for '0e2147483647', and check Spark's error in the sequence test

Review follow-up.

- On the released path, parse_string_to_decimal 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. The zero test covers
  it, and two inputs whose counts don't wrap.
- The sequence overflow test's released-Spark branch uses checkSparkError,
  which checks Spark's INTERNAL_ERROR, exception class and SQLSTATE rather
  than the message.
- The cast audit entry describes SPARK-60118's long digit count: released
  Spark throws for '1e2147483647' and similar strings, the fixed releases
  return NULL or raise NUMERIC_OUT_OF_SUPPORTED_RANGE, and the native cast's
  ANSI error is apache#7188.
- SparkFixes' scaladoc names the two fixes it covers and points to the
  other version checks.

Adapted for branch-1.1, which predates parts of main next to apache#7171's lines:

- sequence.rs: branch-1.1's kernel takes the start, stop and step arrays
  and a conversion closure, and its sequence_length flags the overflow
  itself, so the OverflowedLength choice is threaded through that code.
  The new test also checks that a length over the limit is still an
  error with spark_sequence_recomputed, which main's length grid covers.
- string.rs: the zero_any_exponent change and its test. The SPARK-60119
  comment rewording is dropped, since branch-1.1 doesn't have the
  scale-adjustment comments it edits.
- planner.rs: branch-1.1's SparkCastOptions::new_with_version still takes
  allow_incompat.
- The contributor-guide audit entries are dropped, since that guide isn't
  published from release branches and they describe main's code.
  SparkFixes' scaladoc doesn't mention ArraySetSupport.normalizesFloats,
  which branch-1.1 doesn't have.
- Import lists keep branch-1.1's names plus the ones apache#7171 adds.

(cherry picked from commit 1dec4c0)

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @snmvaughan

@andygrove
andygrove marked this pull request as ready for review October 27, 2026 22:48
@andygrove
andygrove merged commit dea84f9 into apache:branch-1.1 Oct 27, 2026
109 of 110 checks passed
snmvaughan added a commit to snmvaughan/arrow-datafusion-comet that referenced this pull request Oct 28, 2026
Resolves the import conflict with apache#7266 in CometArrayExpressionSuite: the
serde import takes both sides' names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants