[CALCITE-5446] Add support for TIMESTAMP WITH LOCAL TIME ZONE #209
Conversation
| @@ -150,26 +151,34 @@ protected Accessor createAccessor(ColumnMetaData columnMetaData, | |||
| throw new AssertionError("bad " + columnMetaData.type.rep); | |||
| } | |||
| case Types.TIME: | |||
| // TIME WITH LOCAL TIME ZONE is a standard ISO type without proper JDBC support. | |||
| // It represents a global instant in time, as opposed to local clock parameters. | |||
| boolean fixedInstant = | |||
There was a problem hiding this comment.
'fixedInstant' is a tautology. Call it 'instant' (which is Joda time terminology)
| @@ -1130,18 +1165,20 @@ static class DateAccessor extends ObjectAccessor { | |||
| */ | |||
| static class TimeAccessor extends ObjectAccessor { | |||
| private final Calendar localCalendar; | |||
| private final boolean fixedInstant; | |||
There was a problem hiding this comment.
Remove the fixedInstant field and create a separate subclass. Javadoc says 'Accessor that assumes that the underlying value is a TIME' which is a lie if you make the class serve double duty for TIME and TIME_WITH_LOCAL_TIME_ZONE.
Instant and localtime are so confusingly similar we just cannot risk storing them in the same field.
Same comments apply to the TimestampAccessor; create a class TimestampLtzAccessor.
| TimestampFromUtilDateAccessor(Getter getter, | ||
| Calendar localCalendar) { | ||
| TimestampFromUtilDateAccessor( | ||
| Getter getter, Calendar localCalendar, boolean fixedInstant) { | ||
| super(getter); |
There was a problem hiding this comment.
This seems impossible. How could a java.util.Date represent anything but an instant?
| } | ||
|
|
||
| /** | ||
| * Test {@code getTime()} returns the same value as the input time for the local calendar. | ||
| * Test {@code getTime()} returns the same value as the input time for the connection default | ||
| * calendar. |
There was a problem hiding this comment.
Is there a bug in Avatica? If so, say what it is. If not, why are you changing/removing tests?
There was a problem hiding this comment.
This was just me trying to reconcile the test description with the logic therein, though I realize now that I didn't do a great job of it. The test says "for the local calendar", but it's unclear if that means localCalendar (which is not involved in this test at all), or UTC (which in most cases would not be accurately described as "the local calendar"). Upon closer inspection I can appreciate that this test case is really meant to test that no adjustment occurs when the explicitly provided calendar is either UTC or null. I'm working to clean up these tests in general, and in this case I think the I should've just updated the javadoc.
| @Test public void testTimeWithCalendar() throws SQLException { | ||
| value = new Time(0L); | ||
|
|
||
| final TimeZone minusFiveZone = TimeZone.getTimeZone("GMT-5:00"); | ||
| final Calendar minusFiveCal = Calendar.getInstance(minusFiveZone, Locale.ROOT); | ||
| assertThat(instance.getTime(minusFiveCal).getTime(), | ||
| assertThat( |
There was a problem hiding this comment.
reformatting isn't helpful
| */ | ||
| @Test public void testTimeWithCalendar() throws SQLException { | ||
| final int offset = localCalendar.getTimeZone().getOffset(0); |
There was a problem hiding this comment.
so many test changes without any justification. I'm giving up.
There was a problem hiding this comment.
Ya, this is basically how I was feeling when I organized that meeting to look into parallelizing this. I think I have to just do the timestamps separately from the times, and that's probably as good as it's going to get in terms of breaking this up.
F21
force-pushed
the
main
branch
2 times, most recently
from
b97b819
to
4c0999b
Compare
Supersedes #207.
Based on #208, which should be merged first.