Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-5446] Add support for TIMESTAMP WITH LOCAL TIME ZONE #209

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wnob
Copy link
Contributor

@wnob wnob commented Feb 17, 2023

Supersedes #207.

Based on #208, which should be merged first.

@@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug in Avatica? If so, say what it is. If not, why are you changing/removing tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reformatting isn't helpful

*/
@Test public void testTimeWithCalendar() throws SQLException {
final int offset = localCalendar.getTimeZone().getOffset(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so many test changes without any justification. I'm giving up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants