[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 6 commits into
Conversation
…recision for TIME/TIMESTAMP literals
| @Override public void reduce(RexBuilder rexBuilder, List<RexNode> constExps, | ||
| List<RexNode> reducedValues) { | ||
| assert reducedValues.isEmpty(); | ||
| final List<RexNode> exps = new ArrayList<>(); |
There was a problem hiding this comment.
what happens here? you are only reducing expressions which are already literals?
This seems to be some kind of regression - other constant expressions are not reduced?
There was a problem hiding this comment.
I'm copying literals through unchanged, and compiling/reducing only non-literal constant expressions.
There was a problem hiding this comment.
This deserves a comment.
There was a problem hiding this comment.
Done. I added a comment explaining that RexLiteral inputs are already reduced, so reduce keeps them as Rex values instead of round-tripping them through generated code.
|
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
| @Override public void reduce(RexBuilder rexBuilder, List<RexNode> constExps, | ||
| List<RexNode> reducedValues) { | ||
| assert reducedValues.isEmpty(); | ||
| final List<RexNode> exps = new ArrayList<>(); |
There was a problem hiding this comment.
This deserves a comment.
| 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.
| 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
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.