You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2022/11/15 17:00:09 UTC

[GitHub] [calcite-avatica] freastro opened a new pull request, #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

freastro opened a new pull request, #197:
URL: https://github.com/apache/calcite-avatica/pull/197

   This changes the unix timestamp to Date/Time/Timestamp conversion in `AbstractCursor` to convert between the ISO calendar system and the standard Gregorian calendar. This ensures that unix timestamps produced by `DateTimeUtils` are correctly converted to `java.sql.Date/Time/Timestamp` objects.


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] freastro commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
freastro commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1042559515


##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {

Review Comment:
   Thanks for the suggestion. I've added a javadoc comment to every method and class.



##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;
+
+  @Before
+  public void before() {
+    final AbstractCursor.Getter getter = new LocalGetter();
+    localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
+    instance = new AbstractCursor.TimestampFromUtilDateAccessor(getter, localCalendar);
+  }
+
+  @Test
+  public void testTimestamp() throws SQLException {
+    value = new Timestamp(0L);

Review Comment:
   I've updated every test as requested, except for removing `public` which seems to break the tests.



##########
core/src/test/java/org/apache/calcite/avatica/util/DateAccessorTest.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;

Review Comment:
   Thanks for the suggestion. I like to push common test code to the `@Before` method. I think it makes the tests more readable.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] asolimando commented on pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
asolimando commented on PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#issuecomment-1335420683

   @freastro, tests should pass in all TZs, you cannot assume a specific one so you can't hardcode your expected values, you need to derive them dynamically, see if [CALCITE-4793](https://issues.apache.org/jira/browse/CALCITE-4793) can help


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] julianhyde commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1042567249


##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {

Review Comment:
   Thank you. I think that javadoc on methods is optional - especially if the methods are private/package-protected and have descriptive names, and that applies to many test methods - but the javadoc on classes is essential because once you know the purpose of the class you can often figure out the purpose of the methods.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] asfgit closed pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number
URL: https://github.com/apache/calcite-avatica/pull/197


-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] asolimando commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
asolimando commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1041992735


##########
core/src/test/java/org/apache/calcite/avatica/util/DateAccessorTest.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;

Review Comment:
   Thanks @julianhyde for the explanation, since it's safe I leave it up to @freastro to decide what's best here



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] julianhyde commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1041634873


##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;
+
+  @Before
+  public void before() {
+    final AbstractCursor.Getter getter = new LocalGetter();
+    localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
+    instance = new AbstractCursor.TimestampFromUtilDateAccessor(getter, localCalendar);
+  }
+
+  @Test
+  public void testTimestamp() throws SQLException {
+    value = new Timestamp(0L);

Review Comment:
   Please change
   ```
     @Test
     public void testTimestamp() throws SQLException {
   ```
   
   (No 'public', and \@Test on the same line as 'void'.)
   to
   ```
     @Test void testTimestamp() throws SQLException
   ```



##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {

Review Comment:
   Please ensure that every class (including test classes and inner classes) has a javadoc comment. It will help future maintainers understand the purpose of the class.



##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromNumberAccessorTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromNumberAccessorTest {
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Object value;
+
+  @Before
+  public void before() {
+    final AbstractCursor.Getter getter = new LocalGetter();
+    localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
+    instance = new AbstractCursor.TimestampFromNumberAccessor(getter,
+        localCalendar);
+  }
+
+  @Test
+  public void testDate() throws SQLException {
+    value = 0L;
+    assertThat(instance.getDate(localCalendar),
+        is(Date.valueOf("1970-01-01")));
+
+    value = DateTimeUtils.timestampStringToUnixDate("1500-04-30 12:00:00");
+    assertThat(instance.getDate(localCalendar),
+        is(Timestamp.valueOf("1500-04-30 12:00:00")));
+  }
+
+  @Test
+  public void testDateWithCalendar() throws SQLException {
+    value = 0L;
+
+    final TimeZone minusFiveZone = TimeZone.getTimeZone("GMT-5:00");
+    final Calendar minusFiveCal = Calendar.getInstance(minusFiveZone, Locale.ROOT);
+    assertThat(instance.getDate(minusFiveCal).getTime(),
+        is(5 * DateTimeUtils.MILLIS_PER_HOUR));
+
+    final TimeZone plusFiveZone = TimeZone.getTimeZone("GMT+5:00");
+    final Calendar plusFiveCal = Calendar.getInstance(plusFiveZone, Locale.ROOT);
+    assertThat(instance.getDate(plusFiveCal).getTime(),
+        is(-5 * DateTimeUtils.MILLIS_PER_HOUR));
+  }
+
+  @Test
+  public void testDateWithNullCalendar() throws SQLException {
+    value = 0;
+    assertThat(instance.getDate(null), is(new Date(0L)));
+  }
+
+  @Test
+  public void testString() throws SQLException {
+    value = 0;
+    assertThat(instance.getString(), is("1970-01-01 00:00:00"));
+
+    value = DateTimeUtils.timestampStringToUnixDate("2014-09-30 15:28:27.356");
+    assertThat(instance.getString(), is("2014-09-30 15:28:27"));
+
+    value = DateTimeUtils.timestampStringToUnixDate("1500-04-30 12:00:00.123");
+    assertThat(instance.getString(), is("1500-04-30 12:00:00"));
+  }
+
+  @Test
+  public void testStringWithGregorianShift() throws SQLException {
+    for (int i = 4; i <= 15; ++i) {
+      final String str = String.format(Locale.ROOT, "1582-10-%02d 00:00:00", i);
+      value = DateTimeUtils.timestampStringToUnixDate(str);
+      assertThat(instance.getString(), is(str));
+    }
+  }
+
+  @Test
+  public void testStringWithUtc() throws SQLException {
+    localCalendar.setTimeZone(TimeZone.getTimeZone("UTC"));
+
+    value = 0L;
+    assertThat(instance.getString(), is("1970-01-01 00:00:00"));
+
+    value = 1412090907356L;  // 2014-09-30 15:28:27.356 UTC
+    assertThat(instance.getString(), is("2014-09-30 15:28:27"));
+
+    value = -14821444799877L;  // 1500-04-30 12:00:00.123
+    assertThat(instance.getString(), is("1500-04-30 12:00:00"));
+  }
+
+  @Test
+  public void testStringWithAnsiDateRange() throws SQLException {
+    for (int i = 1; i <= 9999; ++i) {
+      final String str = String.format(Locale.ROOT, "%04d-01-01 00:00:00", i);
+      value = DateTimeUtils.timestampStringToUnixDate(str);
+      assertThat(instance.getString(), is(str));
+    }
+  }
+
+  @Test
+  public void testTime() throws SQLException {
+    value = 0L;
+    assertThat(instance.getTime(localCalendar).toString(), is("00:00:00"));
+
+    value = DateTimeUtils.timestampStringToUnixDate("2014-09-30 15:28:27.356");
+    assertThat(instance.getTime(localCalendar).toString(), is("15:28:27"));
+  }
+
+  @Test
+  public void testTimeWithCalendar() throws SQLException {
+    final int offset = localCalendar.getTimeZone().getOffset(0);
+    final TimeZone east = new SimpleTimeZone(
+        offset + (int) DateTimeUtils.MILLIS_PER_HOUR,
+        "EAST");
+    final TimeZone west = new SimpleTimeZone(
+        offset - (int) DateTimeUtils.MILLIS_PER_HOUR,
+        "WEST");
+
+    value = 0;
+    assertThat(instance.getTime(Calendar.getInstance(east, Locale.ROOT)),
+        is(Timestamp.valueOf("1969-12-31 23:00:00")));
+    assertThat(instance.getTime(Calendar.getInstance(west, Locale.ROOT)),
+        is(Timestamp.valueOf("1970-01-01 01:00:00")));
+  }
+
+  @Test
+  public void testTimeWithNullCalendar() throws SQLException {
+    value = 0;
+    assertThat(instance.getTime(null), is(new Time(0L)));
+  }
+
+  @Test
+  public void testTimestamp() throws SQLException {
+    value = 0L;
+    assertThat(instance.getTimestamp(localCalendar),
+        is(Timestamp.valueOf("1970-01-01 00:00:00.0")));
+
+    value = DateTimeUtils.timestampStringToUnixDate("2014-09-30 15:28:27.356");
+    assertThat(instance.getTimestamp(localCalendar),
+        is(Timestamp.valueOf("2014-09-30 15:28:27.356")));
+
+    value = DateTimeUtils.timestampStringToUnixDate("1500-04-30 12:00:00");
+    assertThat(instance.getTimestamp(localCalendar),
+        is(Timestamp.valueOf("1500-04-30 12:00:00.0")));
+  }
+
+  @Test
+  public void testTimestampWithGregorianShift() throws SQLException {
+    value = DateTimeUtils.timestampStringToUnixDate("1582-10-04 00:00:00");
+    assertThat(instance.getTimestamp(localCalendar),
+        is(Timestamp.valueOf("1582-10-04 00:00:00.0")));
+
+    value = DateTimeUtils.timestampStringToUnixDate("1582-10-05 00:00:00");
+    assertThat(instance.getTimestamp(localCalendar),
+        is(Timestamp.valueOf("1582-10-15 00:00:00.0")));
+
+    value = DateTimeUtils.timestampStringToUnixDate("1582-10-15 00:00:00");
+    assertThat(instance.getTimestamp(localCalendar),
+        is(Timestamp.valueOf("1582-10-15 00:00:00.0")));
+  }
+
+  @Test
+  public void testTimestampWithAnsiDateRange() throws SQLException {
+    for (int i = 1; i <= 9999; ++i) {
+      final String str = String.format(Locale.ROOT, "%04d-01-01 00:00:00.0", i);
+      value = DateTimeUtils.timestampStringToUnixDate(str);
+      assertThat(instance.getTimestamp(localCalendar),
+          is(Timestamp.valueOf(str)));
+    }
+  }
+
+  @Test
+  public void testTimestampWithCalendar() throws SQLException {
+    final int offset = localCalendar.getTimeZone().getOffset(0);
+    final TimeZone east = new SimpleTimeZone(
+        offset + (int) DateTimeUtils.MILLIS_PER_HOUR,
+        "EAST");
+    final TimeZone west = new SimpleTimeZone(
+        offset - (int) DateTimeUtils.MILLIS_PER_HOUR,
+        "WEST");
+
+    value = 0;
+    assertThat(instance.getTimestamp(Calendar.getInstance(east, Locale.ROOT)),
+        is(Timestamp.valueOf("1969-12-31 23:00:00.0")));
+    assertThat(instance.getTimestamp(Calendar.getInstance(west, Locale.ROOT)),
+        is(Timestamp.valueOf("1970-01-01 01:00:00.0")));
+  }
+
+  @Test
+  public void testTimestampWithNullCalendar() throws SQLException {
+    value = 0;
+    assertThat(instance.getTimestamp(null).getTime(),
+        is(0L));
+  }
+
+  private class LocalGetter implements AbstractCursor.Getter {
+    @Override
+    public Object getObject() {
+      return value;
+    }
+
+    @Override
+    public boolean wasNull() {

Review Comment:
   \@Override should be on the same line as 'public'



##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -908,11 +887,16 @@ private byte[] getBase64Decoded() throws SQLException {
    * Accessor that assumes that the underlying value is a DATE,
    * in its default representation {@code int};
    * corresponds to {@link java.sql.Types#DATE}.
+   *
+   * <p>For backwards compatibility, timezone offsets are calculated in relation to the local
+   * timezone instead of to UTC. Providing the default calendar will return the underlying value
+   * unmodified. This differs from {@link DateAccessor} which uses the timezone offset from UTC.
+   * </p>

Review Comment:
   We generally don't use `</p>` when writing javadoc. Only `<p>`, after a blank line, as you have done already.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] asolimando commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
asolimando commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1041953622


##########
core/src/test/java/org/apache/calcite/avatica/util/TimeFromNumberAccessorTest.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimeFromNumberAccessorTest {
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Object value;

Review Comment:
   shared r/w variable across tests



##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;

Review Comment:
   shared r/w variable across tests



##########
core/src/test/java/org/apache/calcite/avatica/util/TimeAccessorTest.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimeAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Time value;

Review Comment:
   Shared variable also here, if we decide we want to play it safe, let's change this too



##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampAccessorTest.java:
##########
@@ -0,0 +1,216 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.*;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Timestamp value;

Review Comment:
   shared r/w variable across tests



##########
core/src/test/java/org/apache/calcite/avatica/util/DateAccessorTest.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;
+
+  @Before
+  public void before() {
+    final AbstractCursor.Getter getter = new LocalGetter();
+    localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
+    instance = new AbstractCursor.DateAccessor(getter, localCalendar);
+  }
+
+  @Test
+  public void testDate() throws SQLException {
+    value = new Date(0L);
+    assertThat(instance.getDate(null), is(value));
+
+    value = Date.valueOf("1970-01-01");
+    assertThat(instance.getDate(UTC), is(value));
+
+    value = Date.valueOf("1500-04-30");
+    assertThat(instance.getDate(UTC), is(value));
+  }
+
+  @Test
+  public void testDateWithCalendar() throws SQLException {
+    value = new Date(0L);
+
+    final TimeZone minusFiveZone = TimeZone.getTimeZone("GMT-5:00");
+    final Calendar minusFiveCal = Calendar.getInstance(minusFiveZone, Locale.ROOT);
+    assertThat(instance.getDate(minusFiveCal).getTime(),
+        is(5 * DateTimeUtils.MILLIS_PER_HOUR));
+
+    final TimeZone plusFiveZone = TimeZone.getTimeZone("GMT+5:00");
+    final Calendar plusFiveCal = Calendar.getInstance(plusFiveZone, Locale.ROOT);
+    assertThat(instance.getDate(plusFiveCal).getTime(),
+        is(-5 * DateTimeUtils.MILLIS_PER_HOUR));
+  }
+
+  /**
+   * Test date range 0001-01-01 to 9999-12-31 required by ANSI SQL.
+   *
+   * <p>This test only uses the UTC time zone because some time zones don't have a January 1st
+   * 12:00am for every year.</p>
+   */
+  @Test
+  public void testStringWithAnsiDateRange() throws SQLException {
+    localCalendar.setTimeZone(UTC.getTimeZone());
+
+    final Calendar utcCal = (Calendar) UTC.clone();
+    utcCal.set(1, Calendar.JANUARY, 1, 0, 0, 0);
+    utcCal.set(Calendar.MILLISECOND, 0);
+
+    for (int i = 2; i <= 9999; ++i) {
+      utcCal.set(Calendar.YEAR, i);
+      value = new Date(utcCal.getTimeInMillis());
+      assertThat(instance.getString(),
+          is(String.format(Locale.ROOT, "%04d-01-01", i)));
+    }
+  }
+
+  @Test
+  public void testStringWithLocalTimeZone() throws SQLException {
+    value = Date.valueOf("1970-01-01");
+    assertThat(instance.getString(), is("1970-01-01"));
+
+    value = Date.valueOf("1500-04-30");
+    assertThat(instance.getString(), is("1500-04-30"));
+  }
+
+  @Test
+  public void testStringWithUtc() throws SQLException {
+    localCalendar.setTimeZone(UTC.getTimeZone());
+
+    value = new Date(0L);
+    assertThat(instance.getString(), is("1970-01-01"));
+
+    value = new Date(-14820624000000L /* 1500-04-30 */);
+    assertThat(instance.getString(), is("1500-04-30"));
+  }
+
+  @Test
+  public void testLong() throws SQLException {
+    value = new Date(0L);
+    assertThat(instance.getLong(), is(0L));
+
+    value = Date.valueOf("1500-04-30");
+    final Date longDate = new Date(instance.getLong() * DateTimeUtils.MILLIS_PER_DAY);
+    assertThat(longDate.toString(), is("1500-04-30"));
+  }
+
+  private class LocalGetter implements AbstractCursor.Getter {
+    @Override
+    public Object getObject() {
+      return value;
+    }
+
+    @Override
+    public boolean wasNull() {
+      return value == null;
+    }

Review Comment:
   This will have to be changed if we finally move away from the shared `value` variable



##########
core/src/test/java/org/apache/calcite/avatica/util/DateAccessorTest.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;

Review Comment:
   I am OK with the previous two because they are initialized in `before()`, but I am concerned about test parallelism in gradle messing things up with the shared `Date value`, which is re-written all over the place.
   
   If you share my doubt, it's safer to move this variable as a local variable in each individual test using it.



##########
core/src/test/java/org/apache/calcite/avatica/util/DateFromNumberAccessorTest.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DateFromNumberAccessorTest {
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Object value;

Review Comment:
   Same consideration as for `Date value` in the other test class



##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromNumberAccessorTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromNumberAccessorTest {
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Object value;

Review Comment:
   shared r/w variable across tests



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] julianhyde commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1041977848


##########
core/src/test/java/org/apache/calcite/avatica/util/DateAccessorTest.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.Date;
+import java.sql.SQLException;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;

Review Comment:
   I think it's OK. If a test class has 5 test methods, then junit will create 5 instances of the class. Whether the tests are run in parallel or series, the instance variables won't conflict.
   
   The main reason to set variables in \@Before and clear them in \@After is to prevent excessive memory leaks (and to free resources such as DB connections after the test has finished).
   
   My personal style is to put everything on the stack of the test method, because I am suspicious of non-final fields. But I don't expect everyone to follow my style.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [calcite-avatica] freastro commented on a diff in pull request #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number

Posted by GitBox <gi...@apache.org>.
freastro commented on code in PR #197:
URL: https://github.com/apache/calcite-avatica/pull/197#discussion_r1042355103


##########
core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.avatica.util;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.sql.SQLException;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+
+import static org.apache.calcite.avatica.util.DateTimeUtils.MILLIS_PER_HOUR;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class TimestampFromUtilDateAccessorTest {
+
+  private static final Calendar UTC =
+      Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
+
+  private Cursor.Accessor instance;
+  private Calendar localCalendar;
+  private Date value;
+
+  @Before
+  public void before() {
+    final AbstractCursor.Getter getter = new LocalGetter();
+    localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
+    instance = new AbstractCursor.TimestampFromUtilDateAccessor(getter, localCalendar);
+  }
+
+  @Test
+  public void testTimestamp() throws SQLException {
+    value = new Timestamp(0L);

Review Comment:
   You're probably thinking of JUnit 5? Avatica is still on JUnit 4 which throws the exception below if the method is not public.
   ```
   Method testTimestamp() should be public
   java.lang.Exception: Method testTimestamp() should be public
   ```



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org