Repository navigation
fix(isthmus)!: validate unary arithmetic option preferences - #1395
Merged
nielspardon merged 10 commits intoOct 9, 2026
Merged
nielspardon merged 10 commits into
nielspardon merged 10 commits into
Conversation
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.
Select and export supported unary arithmetic policies from spec v0.103.0: checked or silent integer negation, silent integer abs, and NAN domain errors for FP64 asin/acos. Reject explicit preferences without a proven native policy rather than dropping them. BREAKING CHANGE: Isthmus rejects unsupported unary arithmetic option preferences instead of silently ignoring them.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe scalar function converter now applies unary arithmetic options for supported arithmetic variants. Tests cover option selection and rejection, Rex execution, and SQL export. ChangesUnary arithmetic options
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The supported unary options appear to round-trip consistently, with no outstanding issue identified that would prevent merging after normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check |  |
The description states the supported unary arithmetic policies, unsupported cases, import and export behavior, validation rules, related issue, and breaking change. It provides the required rationale … |
| Title check | |
The title is a valid Conventional Commit title. It clearly identifies the isthmus scope and the main change: validating unary arithmetic option preferences. |
| 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. |
- Fix all pre-merge checks with AI
✨ 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.
This was referenced Oct 9, 2026
nielspardon
reviewed
Oct 9, 2026
nielspardon
left a comment
nielspardon
left a comment
Member
There was a problem hiding this comment.
Same ask as on #1393 and #1394: please rebase this onto the shared option seam once it lands. This PR changes the same defaultConvert, generateBinding and getSqlOperatorFromSubstraitFunc lines as #1392, #1393 and #1394, and the CHECKED_UNARY_MINUS remap here belongs next to #1392's CHECKED_PLUS/MINUS/MULTIPLY/DIVIDE remap in that seam.
nielspardon
reviewed
Oct 9, 2026
nielspardon
approved these changes
Oct 9, 2026
Member
|
next one needing conflicts resolved |
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
2 participants
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.
Integer negate supports SILENT/ERROR; integer abs supports SILENT. FP64 asin/acos support on_domain_error=NAN. Export these policies and select the first supported preference on import.
Explicit SQRT and transcendental rounding policies, FP32 asin/acos domain policies, and factorial overflow policies remain unsupported. Optionless SQRT retains its POWER conversion. Matching is case-insensitive; undeclared values are rejected before fallback selection.
Part of #1173.
BREAKING CHANGE: Unsupported unary preferences are rejected. Export names SILENT for integer negate/abs and NAN for FP64 asin/acos. Consumers without those policies must reject these plans. Undeclared values fail even with a supported fallback.