julianhyde commented on code in PR #209:
URL: https://github.com/apache/calcite-avatica/pull/209#discussion_r1109424344
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -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 =
Review Comment:
'fixedInstant' is a tautology. Call it 'instant' (which is Joda time
terminology)
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -1130,18 +1165,20 @@ static class DateAccessor extends ObjectAccessor {
*/
static class TimeAccessor extends ObjectAccessor {
private final Calendar localCalendar;
+ private final boolean fixedInstant;
Review 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.
##########
core/src/test/java/org/apache/calcite/avatica/util/TimeAccessorTest.java:
##########
@@ -46,77 +49,74 @@ public class TimeAccessorTest {
*/
@Before public void before() {
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);
+ instance = new AbstractCursor.TimeAccessor(getter, localCalendar, false);
}
/**
- * 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.
Review Comment:
Is there a bug in Avatica? If so, say what it is. If not, why are you
changing/removing tests?
##########
core/src/test/java/org/apache/calcite/avatica/util/TimeAccessorTest.java:
##########
@@ -46,77 +49,74 @@ public class TimeAccessorTest {
*/
@Before public void before() {
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);
+ instance = new AbstractCursor.TimeAccessor(getter, localCalendar, false);
}
/**
- * 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.
*/
@Test public void testTime() throws SQLException {
- value = new Time(0L);
+ value = new Time(12345L);
assertThat(instance.getTime(null), is(value));
-
- value = Time.valueOf("00:00:00");
- assertThat(instance.getTime(UTC), is(value));
-
- value = Time.valueOf("23:59:59");
- assertThat(instance.getTime(UTC), is(value));
}
- /**
- * Test {@code getTime()} handles time zone conversions relative to the
local calendar and not
- * UTC.
- */
+ /** Test {@code getTime()} handles time zone conversions relative to the
provided calendar. */
@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(
Review Comment:
reformatting isn't helpful
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -1241,11 +1280,13 @@ static class TimestampAccessor extends ObjectAccessor {
*/
static class TimestampFromUtilDateAccessor extends ObjectAccessor {
private final Calendar localCalendar;
+ private final boolean fixedInstant;
- TimestampFromUtilDateAccessor(Getter getter,
- Calendar localCalendar) {
+ TimestampFromUtilDateAccessor(
+ Getter getter, Calendar localCalendar, boolean fixedInstant) {
super(getter);
Review Comment:
This seems impossible. How could a `java.util.Date` represent anything but
an instant?
##########
core/src/test/java/org/apache/calcite/avatica/util/TimeFromNumberAccessorTest.java:
##########
@@ -36,112 +35,90 @@
*/
public class TimeFromNumberAccessorTest {
+ private static final Calendar UTC =
+ Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+ // UTC+5:30
+ private static final TimeZone IST_ZONE =
TimeZone.getTimeZone("Asia/Kolkata");
+
private Cursor.Accessor instance;
private Calendar localCalendar;
- private Object value;
+ private Long value;
/**
* Setup test environment by creating a {@link
AbstractCursor.TimeFromNumberAccessor} that reads
* from the instance variable {@code value}.
*/
@Before public void before() {
final AbstractCursor.Getter getter = new LocalGetter();
- localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
+ localCalendar = Calendar.getInstance(IST_ZONE, Locale.ROOT);
instance = new AbstractCursor.TimeFromNumberAccessor(getter,
- localCalendar);
+ localCalendar, false);
}
/**
- * Test {@code getString()} returns the same value as the input time.
- */
- @Test public void testString() throws SQLException {
- value = 0;
- assertThat(instance.getString(), is("00:00:00"));
-
- value = DateTimeUtils.MILLIS_PER_DAY - 1000;
- assertThat(instance.getString(), is("23:59:59"));
- }
-
- /**
- * Test {@code getTime()} returns the same value as the input time for the
local calendar.
+ * Test {@code getTime()} and {@code getTimestamp()} return the same instant
as the input time for
+ * the connection default calendar.
*/
@Test public void testTime() throws SQLException {
- value = 0;
- assertThat(instance.getTime(localCalendar), is(Time.valueOf("00:00:00")));
+ value = 12345L;
- value = DateTimeUtils.MILLIS_PER_DAY - 1000;
- assertThat(instance.getTime(localCalendar), is(Time.valueOf("23:59:59")));
+ assertThat(instance.getTime(null), is(new Time(value)));
+ assertThat(instance.getTimestamp(null), is(new Timestamp(value)));
}
/**
- * Test {@code getTime()} handles time zone conversions relative to the
local calendar and not
- * UTC.
+ * Test {@code getTime()} and {@code getTimestamp()} handle time zone
conversions relative to the
+ * provided calendar.
*/
@Test public void testTimeWithCalendar() throws SQLException {
- final int offset = localCalendar.getTimeZone().getOffset(0);
Review Comment:
so many test changes without any justification. I'm giving up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
