fix: align +, -, *, / and try_* with Spark ANSI arithmetic semantics#2220
Closed
davidlghellin wants to merge 9 commits into
Closed
fix: align +, -, *, / and try_* with Spark ANSI arithmetic semantics#2220davidlghellin wants to merge 9 commits into
davidlghellin wants to merge 9 commits into
Conversation
Gold Data ReportNotes
Commit Information
Summary
DetailsGold Data Metrics
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns Sail’s arithmetic operators (+, -, *, /) and try_* variants with Spark 4.1.1 semantics, including ANSI overflow/div-by-zero behavior, decimal precision/scale rules, and several Spark-specific coercion cases, while ensuring the new UDFs remain serializable for distributed execution.
Changes:
- Consolidates arithmetic behavior into unified Spark-style UDFs (
SparkAdd,SparkSubtract,SparkMultiply,SparkDivide) with{ ansi_mode, safe }controlling ANSI vstry_*semantics. - Updates the expression resolver’s arithmetic planning to route overflow-prone/decimal cases through the new UDFs and adds Spark-specific coercions (string-to-numeric, float×decimal promotion, decimal+int-literal narrowing,
DATE - DATEinterval). - Reworks/expands test coverage with new feature files and regenerated plan/gold snapshots; updates proto + codec for remote execution of the new UDFs.
Reviewed changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| python/pysail/tests/spark/function/test_try_multiply.txt | Removes legacy doctest coverage (migrated to feature tests). |
| python/pysail/tests/spark/function/test_try_divide.txt | Removes legacy doctest coverage (migrated to feature tests). |
| python/pysail/tests/spark/function/test_try_add.txt | Removes legacy doctest coverage (migrated to feature tests). |
| python/pysail/tests/spark/function/features/try_subtract.feature | Adds BDD coverage for try_subtract across float/decimal/overflow behavior. |
| python/pysail/tests/spark/function/features/try_multiply.feature | Adds BDD coverage for try_multiply, including interval cases migrated from doctest. |
| python/pysail/tests/spark/function/features/try_divide.feature | Adds BDD coverage for try_divide, including float/decimal + migrated integer/interval cases. |
| python/pysail/tests/spark/function/features/try_add.feature | Adds BDD coverage for try_add, including migrated integer/date/interval/timestamp scenarios. |
| python/pysail/tests/spark/function/features/transform.feature | Updates overflow expectations now that ANSI arithmetic in lambdas is implemented. |
| python/pysail/tests/spark/function/features/divide_by_zero.feature | Adds multi-row div-by-zero behavior and interval div-by-zero semantics. |
| python/pysail/tests/spark/function/features/arithmetic_overflow.feature | Introduces explicit ANSI overflow semantics tests and subquery-guard plan snapshots. |
| python/pysail/tests/spark/function/features/arithmetic_coercion.feature | Adds coercion/regression coverage for Spark-specific arithmetic type rules. |
| python/pysail/tests/spark/function/features/aggregate.feature | Updates expected behavior for ANSI overflow inside aggregate lambdas. |
| python/pysail/tests/spark/function/snapshots/features/arithmetic_overflow.yaml | Adds plan snapshots for guarded vs unguarded ANSI arithmetic cases. |
| python/pysail/tests/spark/delta/snapshots/features/delete.yaml | Updates expected plans where arithmetic is now expressed via spark_add. |
| python/pysail/tests/spark/catalog/snapshots/features/system.yaml | Updates expected plan formatting to reflect spark_add in filters. |
| python/pysail/tests/spark/snapshots/test_tpch.plan.yaml | Regenerates plan snapshots reflecting UDF-based arithmetic rewrites. |
| python/pysail/tests/spark/snapshots/test_clickbench.plan.yaml | Regenerates plan snapshots reflecting UDF-based arithmetic rewrites. |
| crates/sail-spark-connect/tests/gold_data/function/datetime.json | Updates expected error payload to reflect UDF-wrapped arithmetic in plans. |
| crates/sail-plan/src/resolver/expression/mod.rs | Adjusts resolver test to disable ANSI so arithmetic stays as BinaryExpr for name-resolution focus. |
| crates/sail-plan/src/resolver/command/write.rs | Switches delta identity-column arithmetic from retired SparkTry* UDFs to unified UDFs. |
| crates/sail-plan/src/function/scalar/math.rs | Implements Spark-aware arithmetic planning (ANSI overflow routing, coercions, DATE - DATE, subquery guard). |
| crates/sail-function/src/scalar/math/utils/try_op.rs | Adds shared helpers for Spark try_* per-element NULLing behavior and type coercion helpers. |
| crates/sail-function/src/scalar/math/utils/decimal.rs | Implements Spark decimal precision/scale adjustment and Spark-style decimal multiply/divide kernels. |
| crates/sail-function/src/scalar/math/spark_try_subtract.rs | Extends legacy SparkTrySubtract to accept float/decimal using shared checked-kernel logic. |
| crates/sail-function/src/scalar/math/spark_try_mult.rs | Extends legacy SparkTryMult to accept float/decimal using shared checked-kernel logic. |
| crates/sail-function/src/scalar/math/spark_try_div.rs | Removes retired SparkTryDiv in favor of unified SparkDivide. |
| crates/sail-function/src/scalar/math/spark_try_add.rs | Extends legacy SparkTryAdd to accept float/decimal using shared checked-kernel logic. |
| crates/sail-function/src/scalar/math/spark_subtract.rs | Adds unified Spark - / try_subtract implementation with ANSI + safe modes. |
| crates/sail-function/src/scalar/math/spark_multiply.rs | Adds unified Spark * / try_multiply implementation including Spark decimal precision-loss rules. |
| crates/sail-function/src/scalar/math/spark_divide.rs | Adds unified Spark / / try_divide implementation (double promotion, decimal rules, interval rules, ANSI div-by-zero). |
| crates/sail-function/src/scalar/math/spark_add.rs | Adds unified Spark + / try_add implementation with ANSI overflow and decimal overflow semantics. |
| crates/sail-function/src/scalar/math/mod.rs | Registers new arithmetic UDF modules and removes retired ones. |
| crates/sail-execution/src/proto/codec.rs | Adds serialization/deserialization support for new arithmetic UDFs for cluster execution. |
| crates/sail-execution/proto/sail/plan/physical.proto | Extends physical plan proto to encode the new arithmetic UDF variants. |
Comment on lines
+124
to
+130
| [left, right] if is_float_or_decimal(left) || is_float_or_decimal(right) => { | ||
| if is_float(left) || is_float(right) { | ||
| Ok(DataType::Float64) | ||
| } else { | ||
| arith_result_type(left, Operator::Divide, right) | ||
| } | ||
| } |
Comment on lines
+163
to
+176
| let has_decimal = matches!(left, DataType::Decimal128(..) | DataType::Decimal256(..)) | ||
| || matches!(right, DataType::Decimal128(..) | DataType::Decimal256(..)); | ||
| if has_decimal && !is_float(left) && !is_float(right) { | ||
| // Keep two decimals as-is so their individual scales reach the divide | ||
| // kernel; a decimal/integral pair coerces the integer to decimal. | ||
| if matches!(left, DataType::Decimal128(..)) && matches!(right, DataType::Decimal128(..)) | ||
| { | ||
| return Ok(vec![left.clone(), right.clone()]); | ||
| } | ||
| let (l, r) = arith_input_types(left, Operator::Divide, right)?; | ||
| return Ok(vec![l, r]); | ||
| } | ||
| Ok(vec![DataType::Float64, DataType::Float64]) | ||
| } |
Spark 3.5.7 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff--- before.txt 2026-07-08 17:00:43.740974587 +0000
+++ after.txt 2026-07-08 17:00:44.011977405 +0000
@@ -738 +737,0 @@
-pyspark/sql/tests/connect/test_parity_arrow.py::ArrowParityTests::test_toPandas_error
@@ -920 +918,0 @@
-pyspark/sql/tests/connect/test_parity_errors.py::ErrorsParityTests::test_arithmetic_exceptionFailed Tests |
Spark 4.1.1 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff--- before.txt 2026-07-08 17:02:56.739477349 +0000
+++ after.txt 2026-07-08 17:02:57.478477918 +0000
@@ -620 +619,0 @@
-pyspark/sql/tests/connect/arrow/test_parity_arrow.py::ArrowParityTests::test_toArrow_error
@@ -632 +630,0 @@
-pyspark/sql/tests/connect/arrow/test_parity_arrow.py::ArrowParityTests::test_toPandas_error
@@ -1715 +1712,0 @@
-pyspark/sql/tests/connect/test_parity_errors.py::ErrorsParityTests::test_arithmetic_exceptionFailed Tests(truncated) |
Ibis Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests |
davidlghellin
marked this pull request as draft
July 8, 2026 17:04
Contributor
|
@davidlghellin it may be better to start with an analyzer rule. Using a UDF will lead to a large performance penalty. Perhaps you want to pick up my work here? |
Contributor
Author
Thanks @shehabgamin — good point on the UDF perf cost. I'll review #2137 and the analyzer-rule approach properly. Thanks for the pointer! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2198.
Sail's arithmetic operators (
+,-,*,/) and theirtry_*variants hadseveral divergences from Spark: overflow was not ANSI-aware,
try_*rejectedFLOAT/DECIMAL, decimal precision/scale did not follow Spark's rule, and a few
type-coercion cases were wrong. This aligns them with Spark 4.1.1 (all
divergences JVM-validated).
What changed
The regular operators and their
try_*variants are unified into per-operationUDFs
SparkAdd/SparkSubtract/SparkMultiply/SparkDivide { ansi_mode, safe }, following the existingSparkSumconsolidation pattern (one source oftruth per operation). Three modes, in priority order:
safe(try_*) → overflow / div-by-zero → NULL, ANSI-invariant!safe && ansi→ overflow / div-by-zero → ERROR!safe && !ansi→ integers wrap · decimals NULL · div-by-zero NULLThe old
SparkTryAdd/Subtract/Mult/Divstructs are retired. Regular ANSI-offintegral arithmetic stays on the native operator (native wrapping already
matches Spark, and it preserves
BinaryExprfor simplification); only ANSI-onor decimal operands route through the UDF.
Divergences fixed (JVM-validated)
spark.sql.ansi.enabled(raisesARITHMETIC_OVERFLOWunder ANSI on, wraps/NULLs under ANSI off), includingnarrow types (
TINYINT/SMALLINT) which keep their type.try_add/subtract/multiply/dividenow accept FLOAT and DECIMAL.*and/follow Spark's precision/scale rule (adjustPrecisionScale,HALF_UP), e.g.
decimal(10,2) / decimal(10,2)→decimal(23,13).decimal(10,2) * 3→decimal(12,2),decimal(10,2) + 3→decimal(11,2).DATE - DATE→ day-time interval (wasbigint).'5' + 3→ numeric (was a coercion error).float * decimal→double(wasfloat).representations: year-month and day-time raise
INTERVAL_DIVIDED_BY_ZEROinboth ANSI modes;
make_interval(CalendarInterval) raises under ANSI onand returns NULL under ANSI off;
try_dividealways returns NULL.Known limitations (tracked as
@sail-bug)UDF breaks the DataFusion planner's count-bug check when a subquery is nested
inside a correlated subquery body (fails to plan with "does not support
logical expression ScalarSubquery"). As a workaround,
+/-/*fall back tothe native operator when an operand contains a scalar subquery
(
has_scalar_subqueryinmath.rs). Consequence:(SELECT ...) + xwraps onoverflow under ANSI instead of raising. The root fix belongs in the fork's
evaluates_to_null(relates to perf: Implement physical execution of uncorrelated scalar subqueries apache/datafusion#21240 and theScalarSubqueryExecwork in #22566 / #22530); the guard can be dropped oncethat lands.
DATE - DATErendering. Value is correct but renders asINTERVAL … DAY TO SECONDinstead of Spark's… DAYgranularity (Sail stores day-time intervalsas
Duration(µs)).'5' + 3under ANSI on yieldsINTwhere Spark yieldsBIGINT(valuecorrect).
try_divide(day-time interval, 0)has noDurationbranch inSparkDivideyet (pre-existing gap, separate from this change).Testing
arithmetic_overflow,arithmetic_coercion,divide_by_zero,try_{add,subtract,multiply,divide}, plus plan-snapshot(
@sail-only) and@sail-bugscenarios for the subquery guard. All newscenarios JVM-validated against Spark 4.1.1.