[CALCITE-7529] Casts between literals and TIME/TIMESTAMP can lose precision beyond milliseconds#4942
[CALCITE-7529] Casts between literals and TIME/TIMESTAMP can lose precision beyond milliseconds#4942zzwqqq wants to merge 9 commits into
Conversation
…recision for TIME/TIMESTAMP literals
|
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
The restricted scope of this PR looks more reasonable, but I do have some questions
| final SqlTimeLiteral timeLiteral = | ||
| SqlParserUtil.parseTimeLiteral(value, pos); | ||
| if (!isSubMillisecondLiteral(timeLiteral, value)) { | ||
| if (timeLiteral.getPrec() <= 0 |
There was a problem hiding this comment.
I understand what 0 means, but negative precisions are less clear.
There's "PRECISION_NOT_SPECIFIED", but I'd rather see an explicit check for that if that's what you are looking for.
There was a problem hiding this comment.
Switched this to check PRECISION_NOT_SPECIFIED explicitly.
| return timestamp == null || !hasSubMillisecondPrecision(timestamp) | ||
| ? null | ||
| : timestamp.toString(precision(literal.getType())); | ||
| if (timestamp == null || literal.getType().getPrecision() <= 3) { |
There was a problem hiding this comment.
Why is 3 hardwired here?
There was a problem hiding this comment.
that 3 was just the millisecond cutoff from the old version. Removed it and now format using the literal type's precision.
I also checked PostgreSQL: concat_ws(',', timestamp '2024-07-06 12:15:48.678') keeps .678(https://onecompiler.com/postgresql/44q5pytxk). After updating the expectation, CalciteSqlOperatorTest.testConcatWSFunc exposed that the VALUES-based path was still dropping it, so I updated the generated TIME/TIMESTAMP-to-character cast to pass through the source precision.
| default: | ||
| return false; | ||
| } | ||
| return SqlTypeUtil.comparePrecision(type.getPrecision(), value.length()) >= 0 |
There was a problem hiding this comment.
please add a comment explaining what happens here
There was a problem hiding this comment.
Added a note here about when this can replace the CAST with a literal.
| } | ||
|
|
||
| private static RelDataTypeFactory highPrecisionTemporalTypeFactory() { | ||
| return new JavaTypeFactoryImpl( |
There was a problem hiding this comment.
Can you run tests with multiple precisions, checking for precisions 1..9 for example in a loop?
There was a problem hiding this comment.
Expanded these to run through precisions 1..9. I did the same for the temporal-to-VARCHAR cases
zzwqqq
changed the title
| String.class, int.class), | ||
| UNIX_DATE_TO_STRING(DateTimeUtils.class, "unixDateToString", int.class), | ||
| UNIX_TIME_TO_STRING(DateTimeUtils.class, "unixTimeToString", int.class), | ||
| UNIX_TIME_TO_STRING_WITH_PRECISION(DateTimeUtils.class, "unixTimeToString", |
There was a problem hiding this comment.
Both these methods assume that the actual data value is expressed as an integer number of milliseconds, so they won't work for precisions higher than 3. Perhaps this is not worsening the problem, but it looks to me like it will need to be fixed at some point again.
There was a problem hiding this comment.
Right. I ran into this while debugging CalciteSqlOperatorTest.testConcatWSFunc. This is still limited by the runtime representation; a lot of the enumerable path uses milliseconds / Calendar internally, so this only preserves up to 3 fractional digits today.
| : null; | ||
| } | ||
|
|
||
| private static boolean isExactFractionalSecondLiteral( |
There was a problem hiding this comment.
can you document what this function is supposed to do?
There was a problem hiding this comment.
Added a short JavaDoc. This checks that the parsed literal has explicit fractional-second precision and still formats back to the original string.
| return temporalLiteral; | ||
| } | ||
|
|
||
| private @Nullable RexNode makeCastFromTemporalLiteralToCharacter( |
There was a problem hiding this comment.
This needs JavaDoc, particularly because it returns null on many paths.
You would not expect that result from the function name.
There was a problem hiding this comment.
Added JavaDoc here, including the null-return cases where the caller keeps the regular CAST.
| default: | ||
| return null; | ||
| } | ||
| // Replace the CAST with a character literal only if the formatted temporal |
There was a problem hiding this comment.
I don't see any cast in this function, so this comment is misleading.
There was a problem hiding this comment.
I reworded this to describe what the helper does directly, without referring to the surrounding CAST.
| /** Converts a TIME or TIMESTAMP literal to a character literal. | ||
| * | ||
| * <p>Returns null if the literal type is not supported, the literal value is | ||
| * unavailable, or the formatted temporal value does not fit in the target |
There was a problem hiding this comment.
Please do not document what the caller does, it's not the job of the callee. Other callers may be added in the future.
What is "value is unavailable"?
There was a problem hiding this comment.
Thanks, I removed the caller behavior from the JavaDoc and made the null-return cases more specific.
2 participants
Jira Link
CALCITE-7529
Changes Proposed
Preserve precision for casts involving high-precision TIME and TIMESTAMP literals.
Generated constant reduction represents TIME and TIMESTAMP values at millisecond precision. When a custom RelDataTypeSystem allows precision greater than 3, literal casts such as CAST('12:34:56.123456' AS TIME(6)) can lose digits beyond milliseconds.
The change keeps directly representable temporal literal casts as Rex literals, and avoids re-reducing existing RexLiterals.
Regression tests cover TIME(6) and TIMESTAMP(6) literal casts, existing temporal literals, and casts to VARCHAR.