[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

[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