[CALCITE-6282] Avatica ignores time precision when returning TIME results #241
Conversation
|
|
||
| TimeFromNumberAccessor(Getter getter, Calendar localCalendar) { | ||
| TimeFromNumberAccessor(Getter getter, Calendar localCalendar, int precision) { | ||
| super(getter, 0); |
There was a problem hiding this comment.
I have a question, should the 2 in Time(2) be scale or precision?
There was a problem hiding this comment.
I will rename it to "scale"
There was a problem hiding this comment.
I guess I have to leave it as precision, since both the columnMetadataFields which originate this value call it precision, and the helper functions that manipulate this field also call it precision (as you point out in your other comment). It would be too confusing to use different names for this value in different parts of the codebase.
I propose we file a new issue if we want to modify the field name to scale, to make sure that we cover all the software layers involved. In the meantime it would be great to merge this PR because it fixes a genuine bug.
| @@ -1220,7 +1227,7 @@ static class TimestampAccessor extends ObjectAccessor { | |||
| } | |||
| final long unix = | |||
| DateTimeUtils.sqlTimestampToUnixTimestamp(timestamp, localCalendar); | |||
| return timestampAsString(unix, null); | |||
| return timestampAsString(unix, null, this.precision); | |||
There was a problem hiding this comment.
For cast(TIME '12:42:25.34' as TIME(2)) case, whether to consider
DateTimeUtils.unixTimeToSqlTime corresponding changes
|
So what's the plan about merging this PR? Do I have to make somehow the CI work? |
|
I would suggest we temporary comment out the tests in Calcite and renable them when it's upgraded to Avatica 1.25.0. See Julian's suggestion: https://lists.apache.org/thread/b7kb393yvj10rbmsxv4bh076bk2ncx58 |
…ults Signed-off-by: Mihai Budiu <[email protected]>
…