fix(isthmus)!: cast operands until the function's declaration binds them - #1348
Merged
nielspardon merged 3 commits intoOct 1, 2026
Merged
Conversation
An integer operand of decimal arithmetic was cast to the least restrictive decimal, whose scale it does not have, so decimal(7,2) * INT multiplied by a decimal(12,2). It now becomes the decimal that holds its type, decimal(10,0) for an INT. Where the operands share a parameter, like the any1 of gte(any1, any1), and do not bind it, they take the least restrictive type exactly; before, decimals of two precisions were left as they were. A char(n) operand of a varchar declaration becomes a varchar(n).
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: substrait-io/substrait-java/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughFunctionConverter now checks multiple matching function declarations and tries alternate operand coercions. It validates declaration argument types and, when resolvable, string result length. Tests cover decimal and character-string operand conversions. ChangesFunction operand coercion
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change improves decimal and string operand binding. No actionable merge-blocking issue remains; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The main risk is compatibility with consumers that depend on the previous generated plan shape. The inspected conversion path remains limited to already supplied function declarations. Downstream execution permissions and deployment-specific exposure were not established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…s them Calcite's least restrictive decimal gives up scale past precision 38, so TPC-H Q6's l_discount, a bare DECIMAL, met 0.03 - 0.01 at decimal(38,0) and the bound became 0. The common decimal is now built from the most integer digits and the largest scale, and operands are left as they are when that needs more than 38 digits.
nielspardon
reviewed
Sep 30, 2026
…ating A signature match took the first variant whatever it bound. Each matching variant is now tried with the operands as they are, with char(n) as varchar(n), and with char and varchar as string where the declaration takes a string, and the first that binds is taken. Binding now also requires a concretely declared argument to have exactly its type, and a string result no shorter than the call's, so CHAR(10) || CHAR(10) binds concat:str rather than a concat:vchar that types it varchar(10). An integer operand's digits come from its Calcite type.
alexandrefimov
changed the title
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.
Some calls had operands their declaration does not bind:
decimal(7,2) * INTcast the integer todecimal(12,2),d BETWEEN 0.99 AND 1.49mixed two decimal precisions under oneany1, andchar(n)or unboundedvarcharreached declarations for another string type. Operands are now cast just far enough to bind, trying each matching variant in turn and skipping one that would truncate a string result, such asconcat:vcharforCHAR(10) || CHAR(10). In the TPC tests, non-binding calls go from 59 to 2.Part of #1117.
BREAKING CHANGE: calls converted from Calcite get different operand casts, and some bind another variant, for example
concat:strforc || cover chars andlike:str_strforLIKEover an unbounded varchar. Consumers matching the old shape have to accept the new ones.