Repository navigation
fix(spark)!: honor decimal overflow options - #1441
alexandrefimov wants to merge 1 commit into
Conversation
Honor ERROR for decimal add, subtract and multiply per expression. BREAKING CHANGE: Decimal add, subtract and multiply now honor overflow=ERROR even with Spark ANSI disabled and reject explicit options they cannot honor.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Pass and apply overflow options spark/src/main/scala/io/substrait/spark/expression/ToSparkExpression.scala, spark/src/main/scala/io/substrait/spark/expression/ToScalarFunction.scala |
The converter passes the full invocation to the resolver. The resolver validates options for decimal add, subtract, and multiply, then uses ANSI evaluation mode when the accepted option includes ERROR. |
Validate decimal overflow behavior spark/src/test/scala/io/substrait/spark/expression/DecimalOverflowOptionsSuite.scala |
Tests cover overflow and safe results in interpreted evaluation and unsafe projection, ANSI-mode behavior without an option, and validation of option names and values. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant ToSparkExpression
participant ToScalarFunction
participant SparkArithmeticExpression
ToSparkExpression->>ToScalarFunction: Pass scalar-function invocation
ToScalarFunction->>ToScalarFunction: Resolve declaration and validate overflow options
ToScalarFunction->>SparkArithmeticExpression: Set ANSI evaluation mode for ERROR
Suggested reviewers: andrew-coleman
Merge Risk: 
68000
-
No PR-introduced issue requiring a fix before merge was established.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change. It uses valid Conventional Commit syntax and correctly marks the breaking change. Description check The description states the rationale, expected behavior, option handling, specification version, linked issue, and breaking change. It is complete and aligned with the repository template. Linked Issues check Issue #1293requires the Spark consumer to honor an explicit overflow=ERROR option for decimal arithmetic, or reject the plan during conversion. The PR meets both parts for add:dec_dec, subtract:dec_d…Out of Scope Changes check The PR changes three files, and each change connects to the linked issue behavior.
- ToScalarFunction.scala: adds the invocation-aware overload and resolveDecimalOverflow. This is the core fix for#1…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Linked Issues check
-
Explanation
Issue
#1293requires the Spark consumer to honor an explicit overflow=ERROR option for decimal arithmetic, or reject the plan during conversion. The PR meets both parts for add:dec_dec, subtract:dec_dec and multiply:dec_dec.Honoring ERROR: ToScalarFunction.getSparkExpressionFromSubstraitFunc(invocation, args) now receives the full ScalarFunctionInvocation. ToSparkExpression.visit passes expr instead of expr.declaration.key. resolveDecimalOverflow then sets evalMode to EvalMode.ANSI on the resulting Add, Subtract or Multiply. The result no longer depends on spark.sql.ansi.enabled.
Rejecting unsupported options: the code throws UnsupportedOperationException when an option name is not overflow, when a value is not declared for the function, or when no value is ERROR. This covers empty preference lists and SILENT or SATURATE-only lists. Name and value matching uses equalsIgnoreCase.
Calls without options: resolveDecimalOverflow returns the expression unchanged, so session behavior is retained (issue
#1293allows consumer choice when the option is omitted).Tests: DecimalOverflowOptionsSuite covers add, subtract and multiply with ANSI on and off. It checks ArithmeticException on overflow through both eval and UnsafeProjection, safe-value retention, the no-option path, case-insensitive matching, rejected preferences, and conflicting repeated options.
Limits: I read the diffs only. I did not compile the code or run the suite. The code assumes the decimal extension declares an overflow option for these three functions. The new tests exercise that path.
Issues
#1173and#1292are not addressed by this PR, and the issue does not require them.
Full details: Out of Scope Changes check
-
Explanation
The PR changes three files, and each change connects to the linked issue behavior.
- ToScalarFunction.scala: adds the invocation-aware overload and resolveDecimalOverflow. This is the core fix for
#1293. - ToSparkExpression.scala: a one-line change that passes the full invocation to the new overload. This is required to make the options reachable.
- DecimalOverflowOptionsSuite.scala: a new test suite for the changed behavior.
Decimal subtract (subtract:dec_dec) is included beyond the add and multiply examples in the issue body. It is the same defect in the same decimal arithmetic extension, and the same code path handles it. The issue title refers to decimal arithmetic generally, so this is a related extension of the fix, not an unrelated change.
The case-insensitive option-name matching goes beyond the literal value matching in the issue. It is part of the same option-validation logic and does not change unrelated behavior.
No unrelated files or behavior changes appear in the diff. The changes do not touch Isthmus (
#1173) or decimal division (#1292). - ToScalarFunction.scala: adds the invocation-aware overload and resolveDecimalOverflow. This is the core fix for
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Honor explicit overflow=ERROR for decimal add, subtract and multiply independently of the Spark session ANSI setting, following spec v0.106.0. Select ERROR from requested preferences using case-insensitive matching. Calls without options retain session behavior.
Closes #1293
BREAKING CHANGE: Decimal add, subtract and multiply now honor overflow=ERROR even with Spark ANSI disabled and reject explicit options they cannot honor.