fix(isthmus)!: honor floating-point arithmetic options by alexandrefimov · Pull Request #1394 · substrait-io/substrait-java

alexandrefimov · 2026-10-08T17:05:09Z

For FP32/FP64 add, subtract, multiply and divide, export rounding=TIE_TO_EVEN. Divide also exports on_domain_error=NAN. Import requires these modes, directly or as a fallback.

Division-by-zero options remain unsupported because the extension's IEEE description and LIMIT behavior differ from Java division. Leave that option unset on export. Matching is case-insensitive; undeclared values are rejected before fallback selection.

Part of #1173.

BREAKING CHANGE: Unsupported floating-point preferences are rejected. Exported arithmetic calls gain rounding=TIE_TO_EVEN; divide also gains on_domain_error=NAN. Consumers without those policies must reject these plans. Undeclared values fail even with a supported fallback.

SQL CONCAT and || now emit null_handling ACCEPT_NULLS for the standard Substrait concat function. Import accepts that behavior, including as a fallback preference, instead of silently discarding options. BREAKING CHANGE: Standard concat calls with null_handling preferences that do not include ACCEPT_NULLS, unknown options, or empty preference lists are rejected. Include ACCEPT_NULLS only when null propagation is acceptable.

Use one option policy for the standard concat, like, replace, starts_with, ends_with, strpos, substring, lower, upper and initcap mappings. Export the behavior implemented by the Calcite operator and reject import preferences that do not permit it. Other extensions and unmapped option policies are unchanged. BREAKING CHANGE: The covered standard string functions reject unknown options, empty preferences and preferences that do not include the mapped Calcite behavior. Allow that behavior only when it is acceptable for the query.

Export and validate TIE_TO_EVEN rounding for native FP32/FP64 add, subtract, multiply and divide from spec v0.103.0. Divide uses NAN for domain errors. Reject explicit division-by-zero preferences while the pinned spec's IEEE description conflicts with IEEE 754 behavior. BREAKING CHANGE: Isthmus rejects unsupported or ambiguous floating-point arithmetic option preferences instead of silently ignoring them.

coderabbitai · 2026-10-08T17:05:31Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: substrait-io/substrait-java/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 27dba7de-f136-4764-bf86-d53f53551804

📥 Commits

Reviewing files that changed from the base of the PR and between e998888 and 71477f4.

📒 Files selected for processing (7)

isthmus/src/main/java/io/substrait/isthmus/expression/FloatingPointFunctionOptions.java
isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java
isthmus/src/test/java/io/substrait/isthmus/DynamicParameterRoundtripTest.java
isthmus/src/test/java/io/substrait/isthmus/FloatingPointArithmeticOptionsTest.java
isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java
isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java
isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: substrait-io/substrait-java/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 8ceda930-94cd-4115-8377-fbc51557df04

📥 Commits

Reviewing files that changed from the base of the PR and between c130b20 and e998888.

📒 Files selected for processing (8)

isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java
isthmus/src/main/java/io/substrait/isthmus/expression/FloatingPointFunctionOptions.java
isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java
isthmus/src/test/java/io/substrait/isthmus/DynamicParameterRoundtripTest.java
isthmus/src/test/java/io/substrait/isthmus/FloatingPointArithmeticOptionsTest.java
isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java
isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java
isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

New Features
- Floating-point addition, subtraction, multiplication, and division now support explicit rounding and domain-error options, with defaults for supported operations.
- Supported floating-point options are preserved when converting expressions between SQL and Substrait.
Bug Fixes
- Unsupported or conflicting floating-point options are rejected instead of being silently applied.

Walkthrough

Floating-point arithmetic options are generated for supported Calcite arithmetic calls and resolved during Substrait-to-Rex conversion. Tests cover option validation, execution, export, and SQL round-trips.

Changes

Floating-point arithmetic options

Layer / File(s)	Summary
Map and validate floating-point options `isthmus/src/main/java/io/substrait/isthmus/expression/FloatingPointFunctionOptions.java`, `isthmus/src/test/java/io/substrait/isthmus/FloatingPointArithmeticOptionsTest.java`	Maps supported fp32 and fp64 arithmetic functions to Calcite operators. Generates `TIE_TO_EVEN` and division `NAN` options for matching calls. Rejects unsupported options and incompatible operators.
Attach options to exported arithmetic calls `isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java`, `isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java`, `isthmus/src/test/java/io/substrait/isthmus/{DynamicParameterRoundtripTest,VirtualTableScanTest,VirtualTableTest,FloatingPointArithmeticOptionsTest}.java`	Builds non-datetime scalar invocations with options derived from Calcite calls. Updates arithmetic test inputs to include `TIE_TO_EVEN` and adds SQL export checks.
Resolve invocation options during Rex conversion `isthmus/src/main/java/io/substrait/isthmus/expression/{ExpressionRexConverter,ScalarFunctionConverter,FloatingPointFunctionOptions}.java`, `isthmus/src/test/java/io/substrait/isthmus/FloatingPointArithmeticOptionsTest.java`	Passes the complete invocation to operator lookup. Resolves supported options and tests their execution, rejection cases, and round-trip preservation.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ExpressionRexConverter
  participant ScalarFunctionConverter
  participant FloatingPointFunctionOptions
  ExpressionRexConverter->>ScalarFunctionConverter: Pass complete scalar-function invocation
  ScalarFunctionConverter->>FloatingPointFunctionOptions: Resolve invocation options for selected operator
  FloatingPointFunctionOptions-->>ScalarFunctionConverter: Return selected operator or reject unsupported options
  ScalarFunctionConverter-->>ExpressionRexConverter: Return resolved operator

Suggested reviewers: nielspardon

Merge Risk: ⚪ Minimal · up to e9988

No actionable merge-blocking issue was established for the supported floating-point arithmetic calls.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 18.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title is a valid Conventional Commit title and clearly identifies the breaking fix for floating-point arithmetic options.
Description check		The description explains the rationale, supported options, rejected options, fallback behavior, issue reference, and BREAKING CHANGE impact. It satisfies the repository template requirements.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Create a new PR

Autopilot · 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

alexandrefimov · 2026-10-08T17:17:30Z

Could you please rerun the editorconfig job?

nielspardon

This PR, #1390, #1392, #1393 and #1395 each set .options(XxxOptions.forCall(...)) on the same line in ScalarFunctionConverter.generateBinding, and the last three also add the same getSqlOperatorFromSubstraitFunc(ScalarFunctionInvocation) overload, so every merge after the first has to recombine them by hand. Could you land one shared seam first — a single overload plus a list of option policies that both directions iterate — and rebase the family onto it?

…ions

nielspardon

Thanks — the promotion fix and the move onto the shared policies look good. Please also extend the BREAKING CHANGE: footer to say that exported FP add/subtract/multiply/divide now carry rounding (and on_domain_error for divide), since that changes the plans Isthmus produces, not just the ones it accepts.

nielspardon · 2026-10-09T13:48:18Z

next one to merge with main with conflicts

alexandrefimov added 4 commits October 8, 2026 18:30

fix(isthmus): validate string options against the selected operator

15983c3

nielspardon mentioned this pull request Oct 9, 2026

fix(isthmus)!: honor decimal arithmetic overflow options #1393

Merged

nielspardon reviewed Oct 9, 2026

View reviewed changes

Comment thread isthmus/src/main/java/io/substrait/isthmus/expression/FloatingPointFunctionOptions.java Outdated

Comment thread isthmus/src/main/java/io/substrait/isthmus/expression/FloatingPointFunctionOptions.java Outdated

nielspardon mentioned this pull request Oct 9, 2026

fix(isthmus)!: validate unary arithmetic option preferences #1395

Merged

alexandrefimov added 3 commits October 9, 2026 10:24

refactor(isthmus): share scalar option policies across both directions

4e90106

Merge shared option policies and preserve promoted floating-point opt…

8efa57e

…ions

Merge main and shared scalar option policies

161b55f

nielspardon reviewed Oct 9, 2026

View reviewed changes

Merge main and address arithmetic option review

a7ff121

nielspardon approved these changes Oct 9, 2026

View reviewed changes

Merge main after integer arithmetic options

78348f2

Merge main after decimal arithmetic options

71477f4

nielspardon merged commit 73232c3 into substrait-io:main Oct 9, 2026
14 checks passed

Navigation Menu

Repository navigation

fix(isthmus)!: honor floating-point arithmetic options - #1394

fix(isthmus)!: honor floating-point arithmetic options#1394
nielspardon merged 10 commits into
substrait-io:mainfrom
alexandrefimov:floating-point-options-1173

alexandrefimov commented Oct 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 8, 2026 •

edited

Loading

Review limit reached

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

alexandrefimov commented Oct 8, 2026

Uh oh!

nielspardon left a comment

Uh oh!

Uh oh!

Uh oh!

nielspardon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nielspardon commented Oct 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alexandrefimov commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

alexandrefimov commented Oct 8, 2026

Uh oh!

nielspardon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nielspardon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nielspardon commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexandrefimov commented Oct 8, 2026 •

edited

Loading

coderabbitai Bot commented Oct 8, 2026 •

edited

Loading

nielspardon commented Oct 9, 2026 •

edited

Loading