[CALCITE-5446] Add support for TIMESTAMP WITH LOCAL TIME ZONE#207
[CALCITE-5446] Add support for TIMESTAMP WITH LOCAL TIME ZONE#207wnob wants to merge 2 commits intoapache:mainfrom
Conversation
| true, (byte) 1, (short) 2, 3, 4L, 5.0f, 6.0d, "testvalue", | ||
| new Date(1476130718123L), new Time(1476130718123L), | ||
| new Timestamp(1476130718123L), | ||
| new Date(DST_INSTANT), new Time(DST_INSTANT), |
There was a problem hiding this comment.
Note:
DST_INSTANT = 1476130718123STANDARD_INSTANT = 1479123123242VALID_TIME = 1476123OVERFLOW_TIME = 147912242
So this is not changing any of the test values; it's just giving them names so that equivalence is more apparent throughout the file.
There was a problem hiding this comment.
When I merge to main, I might split out a 'refactor' commit that does not add or change tests but just moves literals into constant fields. That will reduce confusion for future readers.
| ColumnMetaData.scalar(Types.DATE, "DATE", ColumnMetaData.Rep.PRIMITIVE_INT); | ||
| Array expectedArray = | ||
| new ArrayFactoryImpl(TimeZone.getTimeZone("UTC")).createArray( | ||
| new ArrayFactoryImpl(DateTimeUtils.DEFAULT_ZONE).createArray( |
There was a problem hiding this comment.
This is to work around https://issues.apache.org/jira/browse/CALCITE-5488. Using the default zone instead of UTC for the array factory means that it's only adjusted once, after the array is converted to a result set.
| final AbstractCursor.Getter getter = new LocalGetter(); | ||
| localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT); | ||
| instance = new AbstractCursor.TimeAccessor(getter, localCalendar); | ||
| localCalendar = Calendar.getInstance(IST_ZONE, Locale.ROOT); |
There was a problem hiding this comment.
I think it's better to use a hardcoded time zone than the system default for these unit tests, so adjustment is tested consistently regardless of the host machine.
There was a problem hiding this comment.
In general I agree.
But some of JDBC's behavior is defined to use the local time zone. We need to test that behavior.
There was a problem hiding this comment.
The local time zone is injected at a higher level than inside the getters. The getters are provided with the local time zone at construction time by AbstractCursor, so the logic involving the system default time zone is tested in AvaticaResultSetConversionsTest
F21
force-pushed
the
main
branch
2 times, most recently
from
b97b819 to
4c0999b
Compare
4 participants
I know you already suggested getting rid of the
fixedInstantboolean. I'm going to start teasing that apart now, but I'm looking for feedback on the tests in the mean time, which make up the bulk of this PR. It's a big PR but all the test cases are relatively independent and the test files can be reviewed one at a time.