fix(isthmus)!: derive SUM, AVG and decimal arithmetic types as the extensions declare - #1347
alexandrefimov wants to merge 3 commits into
Conversation
…tensions declare Isthmus typed SUM and AVG as their argument and SUM0 as BIGINT, and left decimal arithmetic to Calcite's defaults, so SUM over decimal(7,2) was typed decimal(7,2) where sum:dec declares decimal(38,2), and SUM over i32 was typed i32 where the declaration is i64. SubstraitTypeSystem now derives the sum, the average and decimal +, -, * and / as the extensions declare them, including the scale a result above precision 38 gives up, and isthmus's SUM, AVG and SUM0 take their types from it. Nothing is cast back: the wider type is what Calcite derives.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: substrait-io/substrait-java/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 (5)
🚧 Files skipped from review as they are similar to previous changes (1)
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
WalkthroughThis change adds Substrait-specific result-type derivation for aggregates and decimal arithmetic. SUM and AVG use derived types. Tests compare these types with extension declarations and converter expectations. ChangesType Inference
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The numeric result-type changes appear ready to merge after normal checks. They intentionally widen aggregate outputs and change decimal arithmetic types, so consumers relying on previous types must account for that breaking change. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to This intentionally changes result widths and decimal scales, so callers relying on previous schemas may need explicit casts. The reviewed paths retain aggregate binding and validation behavior; no introduced security issue was established. Application-level exposure remains unverified. 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 |
|
@coderabbitai resume |
|
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Deriving these in SubstraitTypeSystem is the right shape, and the dec×dec formulas match the extension. The BIGINT operand regression inline is the one to fix first; the rest are smaller.
Please also rework the BREAKING CHANGE footer: not every column widens (a result above precision 38 loses scale), STDDEV/VAR over decimals change too, and the footer should say what consumers do. Something like:
BREAKING CHANGE: isthmus types SUM, SUM0, AVG and decimal + - * / as the extensions declare them: an integer SUM or SUM0 is BIGINT, a floating-point one DOUBLE, and a decimal SUM, SUM0 or AVG is DECIMAL(38, s); STDDEV_* and VAR_* over a decimal become DECIMAL(38, s) as well. A decimal result above precision 38 now gives up scale, so DECIMAL(38,20) * DECIMAL(38,20) is DECIMAL(38,6) rather than DECIMAL(38,38). Cast the results where code relies on the previous types.
SUM, AVG and SUM0 now follow the type system of a custom typeFactory(...), so the validator/cluster split in #1220 fails with type mismatch on the SQL path for them. Nothing to change here, but it makes #1220 more pressing.
|
@coderabbitai resume |
✅ Action performedReviews resumed and review finished. |
… modulus Under this type system decimalOf(BIGINT) is DECIMAL(38,0), so a BIGINT operand made d + b a decimal<38,2> where add:dec_dec declares decimal<22,2>. Only a Java type goes through decimalOf now. Decimal modulus follows modulus:dec_dec too, a decimal SUM keeps Calcite's own DECIMAL(38, s), and SUM0 relies on its superclass, which already infers the sum's type made NOT NULL.
| * An integer operand counts as the decimal that holds its type, the one isthmus casts it to: | ||
| * {@code decimal(10,0)} for an INTEGER and {@code decimal(19,0)} for a BIGINT, whatever the type |
There was a problem hiding this comment.
Drop "the one isthmus casts it to": SELECT d * b casts b to decimal(21,2), not decimal(19,0), and emits decimal<27,2> where multiply:dec_dec declares decimal<29,4> for those arguments. That's the integer-operand gap the description already defers, so only the wording needs to change.
2 participants
Isthmus typed
SUMandAVGas their argument,SUM0asBIGINT, and used Calcite's decimal rules, soSUMoverdecimal(7,2)came outdecimal(7,2)instead of the declareddecimal(38,2), andSUMoveri32stayedi32instead ofi64.SubstraitTypeSystemnow derivesSUM,AVGand decimal+ - * / %as the extensions declare them, and isthmus's aggregates take their types from it. Nothing is cast back.Part of #1117. Decimal arithmetic with an integer operand still differs; that goes with the operand casts, as agreed there.
BREAKING CHANGE: isthmus types SUM, SUM0, AVG and decimal + - * / % as the extensions declare them: an integer SUM or SUM0 is BIGINT, a floating-point one DOUBLE, and a decimal SUM, SUM0 or AVG is DECIMAL(38, s); STDDEV_* and VAR_* over a decimal become DECIMAL(38, s) as well. A decimal result above precision 38 now gives up scale, so DECIMAL(38,20) * DECIMAL(38,20) is DECIMAL(38,6) rather than DECIMAL(38,38). Cast the results where code relies on the previous types.