fix(isthmus)!: convert temporal literals at their declared type - #1121
fix(isthmus)!: convert temporal literals at their declared type#1121alexandrefimov wants to merge 6 commits into
Conversation
|
Two things carried over from the #1120 review, so they do not have to be said twice. The rescale now uses I also checked whether the rounding defect you found in #1120 exists here. It does not, and for a reason worth stating: Calcite hands the literal over already at its declared precision, so there is no narrowing decision on this side. Where narrowing does show, a timestamp floors rather than truncating towards the epoch — While checking it I hit an unrelated, pre-existing bug: a pre-epoch |
|
Self-review pass before this sits waiting, in the shape of the #1120 one. Checked at runtime rather than by reading, and two of the three came back clean:
Also noted while there: #1127 fixes a pre-epoch throw in the same file, and the two do not overlap — that one is inside |
76d6af8 to
cf8a6e5
Compare
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
The TIMESTAMP_TZ diagnosis is right and the fix is real: getMaxPrecision(TIMESTAMP_TZ) is 3 while the type path uses TIMESTAMP_WITH_LOCAL_TIME_ZONE at 6, so precision_timestamp_tz<6> converted as a type but was rejected as a literal. Honouring the declared precision also fixes a real failure — SELECT * FROM (VALUES (TIMESTAMP '2024-01-01 00:00:00')) threw before this change and passes after it.
Emitting whatever precision Calcite declares is the right call, so the gap this opens is on the other side. getTimestampString and createTimeString only handle 0/3/6/9 and throw otherwise, so TIMESTAMP '2024-01-01 00:00:00.12' — which Calcite types TIMESTAMP(2) from the fractional digits written — now produces a plan Isthmus cannot read back. Nothing restricts a Substrait precision to multiples of three: algebra.proto only enumerates 0/3/6/9/12 as examples of what the number means, and this repo's own Spark path already rescales any precision 0–6 since #1128. So those two helpers should generalize rather than the forward direction narrowing back.
Worth rebasing onto main first — #1128, #1136 and #1110 landed after you branched and resolve several things this diff otherwise runs into. Also: the PR body's merge-order note and the sentence narrating the tTimestampTZ change are review notes rather than commit body.
getTimestampString and createTimeString split a value into whole seconds and a nanosecond remainder with truncating division. Before the epoch the value is negative, so truncation leaves a negative remainder, and both TimestampString and TimeString reject that with a bare IllegalArgumentException: a precision_timestamp of -500 at precision 3, which is 1969-12-31 23:59:59.5, did not convert at all. Split with floorDiv/floorMod so the remainder is never negative, and share the split between the two methods rather than repeating it once per supported precision. The set of supported precisions and both failure messages are unchanged.
Found reviewing the change above. A precision_time is a time of day, so a value below zero or a day or more is not one. Neither is reported as such today: a negative value reaches TimeString.fromMillisOfDay and comes back as a corrupt time string (Invalid time format: [00:00:0/]), and a value of a day or more is either Guava's "Hour out of range: [24]" from inside TimeString or, once the millisecond count overflows an int, a silently wrong time - 4346734 seconds converts to 14:22:46. A range check names the value instead, and makes the narrowing to int provably safe.
LiteralConverter.convert already computes the Substrait type of the literal it is handed, then ignored it for the temporal cases: TIME and TIMESTAMP were reported at a fixed precision 6 with the value rescaled to microseconds, and TIMESTAMP_WITH_LOCAL_TIME_ZONE shared the TIMESTAMP branch and came back as a plain precision_timestamp. The type converter maps the same Calcite types to precision_time<P>, precision_timestamp<P> and precision_timestamp_tz<P>, so the two halves of the same conversion disagreed about the same literal. Take the precision from the result type and express the value at it, and route TIMESTAMP_WITH_LOCAL_TIME_ZONE to precisionTimestampTZ. At precision 6 the value arithmetic is what it was; other precisions now round-trip instead of being widened. In the other direction, the maximum-precision check for a precision_timestamp_tz literal looked up SqlTypeName.TIMESTAMP_TZ, which SubstraitTypeSystem does not configure - so it fell through to Calcite's default of 3 and rejected microsecond TZ literals, while the corresponding type conversion accepted them. It now checks TIMESTAMP_WITH_LOCAL_TIME_ZONE, the name everything else TZ-related uses, and reports precision_timestamp_tz rather than precision_timestamp when it throws. BREAKING CHANGE: a Calcite TIME or TIMESTAMP literal now converts to a Substrait literal carrying its own precision rather than always precision 6, so plans Isthmus produces from SQL such as TIMESTAMP '2024-01-01 00:00:00' now declare precision_timestamp<0> with a value in seconds instead of precision_timestamp<6> with a value in microseconds. A TIMESTAMP WITH LOCAL TIME ZONE literal converts to precision_timestamp_tz instead of precision_timestamp.
Replaces the hand-rolled power-of-ten loop, matching what the interval rescale in the sibling change uses, so the two do not drift. Also records why narrowing a pre-epoch timestamp floors rather than truncating towards the epoch: the value is an instant, not a duration, so 1969-12-31 23:59:59.5 at second precision is 23:59:59. The interval conversion narrows in the other direction for the opposite reason.
The breaking-change note is about the plans Isthmus produces from SQL, and nothing asserted that half: a literal written without a fractional part is TIMESTAMP(0) on the Calcite side, so the plan now declares precision_timestamp<0> with a value in seconds rather than precision_timestamp<6> with a value in microseconds.
…hird one Review follow-up. Emitting the precision Calcite declares only works if the reverse direction takes it back, and it did not: getTimestampString and createTimeString handled 0, 3, 6 and 9 and threw for everything else, so TIMESTAMP '2024-01-01 00:00:00.12' -- precision 2, from the fractional digits written -- produced a plan Isthmus could not read. algebra.proto documents what 0, 3, 6, 9 and 12 mean and leaves the field an unbounded int32, so the unit is now 10^-precision for any precision a TimeString or TimestampString carries. Two more from the same review. A temporal type built from a Java class rather than a SQL type name carries PRECISION_NOT_SPECIFIED, and nothing read it as anything: a java.sql.Time field of a reflective schema became precision_time<-1> with the value divided by 10^10, and the timestamp side failed inside Guava. Both the type and the literal now read the sentinel as precision 0, the way Calcite's own RexBuilder.clean does. The precision check was written out six times, identical apart from a SqlTypeName and a hand-typed noun -- which is how the TZ literal ended up checked against the wrong name with the wrong noun in its message. It is now SubstraitTypeSystem.requireSupportedPrecision.
cf8a6e5 to
7fa13d6
Compare
|
Rebased onto #1127 as you suggested, so the first two commits here are that PR and the generalization sits on top of its The reverse direction takes any precision 0-9 now, One correction to something I then wrote myself: |
2 participants
LiteralConverter.convertcomputes the Substrait type of the literal it is handed on its first line, then does not use it in the temporal cases.TIMEandTIMESTAMPwere reported at a fixed precision 6 with the value rescaled to microseconds, andTIMESTAMP_WITH_LOCAL_TIME_ZONEshared theTIMESTAMPbranch and came back as a plainprecision_timestamp.TypeConverter.toSubstraitmaps those same Calcite types toprecision_time<P>,precision_timestamp<P>andprecision_timestamp_tz<P>, so the two halves of one conversion disagreed about the same literal.That is not only a round trip losing the producer's choice — SQL literals carry their own precision, so it is what Isthmus emits:
Take the precision from the result type and express the value at it, and route
TIMESTAMP_WITH_LOCAL_TIME_ZONEtoprecisionTimestampTZ. At precision 6 the arithmetic is unchanged; other precisions round-trip instead of being widened.In the other direction, the maximum-precision check for a
precision_timestamp_tzliteral looked upSqlTypeName.TIMESTAMP_TZ.SubstraitTypeSystemdoes not configure that name — it overridesTIMESTAMP_WITH_LOCAL_TIME_ZONE— so the lookup fell through to Calcite's default of 3 and rejected a microsecond TZ literal, while the corresponding type conversion accepted it. It now checks the name everything else TZ-related uses.Emitting the declared precision only works if the reverse direction takes it back, and it did not:
getTimestampStringandcreateTimeStringhandled 0, 3, 6 and 9 and threw for everything else, soTIMESTAMP '2024-01-01 00:00:00.12'— precision 2, from the fractional digits written — produced a plan Isthmus could not read.algebra.protodocuments what 0, 3, 6, 9 and 12 mean and leaves the field an unboundedint32, so the unit is now 10^-precision at any precision aTimeStringorTimestampStringcarries. Precisions 1, 2, 4 and 5 were listed inExpressionConvertabilityTestas unsupported for both timestamp literals; they moved to the supported side.Two smaller things. A temporal type built from a Java class rather than a SQL type name carries
PRECISION_NOT_SPECIFIED, which nothing read: ajava.sql.Timefield of a reflective schema becameprecision_time<-1>with the value divided by 10^10, and the timestamp side failed inside Guava. Both the type and the literal now read the sentinel as precision 0, the way Calcite's ownRexBuilder.cleandoes. And the precision check, written out six times and identical apart from aSqlTypeNameand a hand-typed noun, is nowSubstraitTypeSystem.requireSupportedPrecision— a static, sinceTypeConverterandExpressionRexConverterhave no common ancestor to hang an instance method on.Built on #1127, which rewrites the same two helpers; its two commits are the first two here.
Closes #1113. Finishes #1114, whose interval third is in #1120.
BREAKING CHANGE: a Calcite
TIMEorTIMESTAMPliteral now converts to a Substrait literal carrying its own precision rather than always precision 6, so a plan built fromTIMESTAMP '2024-01-01 00:00:00'declaresprecision_timestamp<0>with a value in seconds where it previously declaredprecision_timestamp<6>with a value in microseconds. ATIMESTAMP WITH LOCAL TIME ZONEliteral converts toprecision_timestamp_tzinstead ofprecision_timestamp.