fix(isthmus)!: convert precision_time and precision_timestamp up to nanoseconds - #1318
Conversation
…anoseconds SubstraitTypeSystem capped TIME, TIMESTAMP and TIMESTAMP_WITH_LOCAL_TIME_ZONE at microseconds, so a plan carrying a nanosecond precision_timestamp was refused although Calcite 1.42 builds such a type the moment the ceiling allows it, and TimestampString carries the digits. Raise the ceiling to nanoseconds. Precisions 10 to 12, which Substrait allows and Calcite clamps rather than reports, stay refused, naming the bound. Converting back, a value is a 64-bit count of its own unit, so nanoseconds reach only 2262 where a TimestampString reaches 9999: a timestamp outside that range is now reported instead of wrapping. Closes substrait-io#995
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults 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: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughIsthmus now supports precision 9 for ChangesPrecision conversion
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established; this change is ready for normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changed conversion rules have compatibility implications, but the reviewed paths retain precision checks and add bounds checks. No introduced security issue was established. How applications handle conversion failures remains uncertain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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 |
The floored seconds of 1677-09-21 00:12:43 overflow when scaled to nanoseconds although the value fits once the sub-second part is added back, so a negative timestamp borrows that second before scaling. Pin both ends of the range and the nanosecond past each.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Decide what a nanosecond column should do with an out-of-range sentinel before this lands — on a TIMESTAMP(9) column, SELECT * FROM v WHERE ts > TIMESTAMP '9999-12-31 23:59:59' now fails conversion even though the literal has no fractional digits, because Calcite widens it to the column's precision. Everything below is mechanical by comparison. Once the first point is settled the BREAKING CHANGE: footer needs the losing side too, and the body should credit #1298, whose wrap epochUnits actually fixes.
A TIMESTAMP(9) column widens a compared literal to nanoseconds, so a sentinel date outside 1677-2262 has no value of that type, and the conversion reports it, as DuckDB and Arrow do. Also pass Locale.ROOT to the overflow message, cover precisions 7 to 9 in the value round trips, and fix the comment on the date minus interval result.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Thanks, all three round-1 points are addressed. Three smaller ones below; only the first is worth holding the merge for.
…g range A TimestampString spans years 0000 to 9999, and past that DateTimeUtils renders the year modulo 10000, so precision_timestamp<7> 3000000000000000000 became 1476-08-15 05:20:00. The value is now checked the way a precision_time already is. Also documents the IllegalArgumentException of LiteralConverter.convert(RexLiteral) and why the time ceilings stop at 9.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Thanks, all three are in. One last nit below, not worth holding the merge for.
%d formats in the default locale, so under a non-Latin numbering system the precision_time and precision_timestamp messages carried other digits and the exact-match test assertions failed. %s does not.
2 participants
SubstraitTypeSystemcappedTIME,TIMESTAMPandTIMESTAMP_WITH_LOCAL_TIME_ZONEat microseconds, so a nanosecondprecision_timestampwas refused although Calcite 1.42.0 builds such a type the moment the ceiling allows it. Raise it to nanoseconds; 10 to 12, which Calcite clamps rather than reports, stay refused and name the bound.Converting back, a Substrait temporal value is a 64-bit count of its own unit, so nanoseconds reach only 2262 where a
TimestampStringreaches 9999. A timestamp past that is now reported instead of wrapping into a different instant. In the other direction, aprecision_timestampoutside years 0000 to 9999 is reported too; Calcite would render its year modulo 10000.On a
TIMESTAMP(9)column,ts > TIMESTAMP '9999-12-31 23:59:59'no longer converts, because the literal is widened to nanoseconds. DuckDB and Arrow refuse it too.Summary by CodeRabbit
TIME,TIMESTAMP, and timestamp-with-local-time-zone values now support precision up to nanoseconds. Time-with-local-time-zone values retain their existing precision limit.Closes #995
Closes #1298
BREAKING CHANGE: precision_time, precision_timestamp and precision_timestamp_tz convert up to nanoseconds, so plans that were refused now convert and TIME(9) or TIMESTAMP(9) can appear where the type system allowed at most 6. A TIMESTAMP(9) column compared with a literal outside 1677-2262 no longer converts. A precision_timestamp outside years 0000-9999 no longer converts to Calcite.