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/12/02 15:08:19 UTC
[GitHub] [calcite] asolimando commented on a diff in pull request #2995: [CALCITE-5414] Use DateTimeUtils to correctly convert between java.sql types and unix timestamps
asolimando commented on code in PR #2995:
URL: https://github.com/apache/calcite/pull/2995#discussion_r1038191382
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -2006,15 +1997,11 @@ public static long toLong(Timestamp v) {
return toLong(v, LOCAL_TZ);
}
- // mainly intended for java.sql.Timestamp but works for other dates also
- @SuppressWarnings("JavaUtilDate")
- public static long toLong(java.util.Date v, TimeZone timeZone) {
- final long time = v.getTime();
- return time + timeZone.getOffset(time);
+ public static long toLong(Timestamp v, TimeZone timeZone) {
Review Comment:
Please add some javadoc for `public` methods (also below, for instance)
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -2172,7 +2163,11 @@ public static String timeWithLocalTimeZoneToString(int v, TimeZone timeZone) {
/** Converts the internal representation of a SQL TIMESTAMP (long) to the Java
* type used for UDF parameters ({@link java.sql.Timestamp}). */
public static java.sql.Timestamp internalToTimestamp(long v) {
- return new java.sql.Timestamp(v - LOCAL_TZ.getOffset(v));
+ final LocalDateTime dateTime = LocalDateTime.ofEpochSecond(
+ v / DateTimeUtils.MILLIS_PER_SECOND,
+ (int) (v % DateTimeUtils.MILLIS_PER_SECOND * 1000000),
+ ZoneOffset.UTC);
Review Comment:
`UTC` is the right way to go for storing timestamps, can you please just state it explicitly in the method javadoc?
I think it's useful because too many people get tricked by this, we have had countless questions on this exact subject, being vocal in the doc will help.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -974,4 +978,128 @@ private void thereAndBack(byte[] bytes) {
// ok
}
}
+
Review Comment:
I'd rather split these tests into smaller units, basically wherever you have a comment saying "this tests xyz" I'd turn it into a separate `testNameXyz`, so that in case of test failures it's easier to understand at a glance what broke, and tests are generally easier to read since they are shorter.
##########
testkit/src/main/java/org/apache/calcite/test/QuidemTest.java:
##########
@@ -293,6 +293,7 @@ public Connection connect(String name) throws Exception {
.connect();
case "catchall":
return CalciteAssert.that()
+ .with(CalciteConnectionProperty.TIME_ZONE, "UTC")
Review Comment:
I am wondering if this impacts our CI which tests under different timezones (see [this](https://github.com/apache/calcite/blob/687dec0afcfab781f905b16e422bc03bd6b9209e/.github/workflows/main.yml#L142-L165) for instance).
Can you try locally to run with different timezones and see if those tests are affected?
You can use ```./gradlew assemble --no-build-cache cleanTest $yourTestsHere -Duser.timezone=$TZ``` for that.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -974,4 +978,128 @@ private void thereAndBack(byte[] bytes) {
// ok
}
}
+
+ @Test void testSqlDateToUnixDate() {
+ assertThat(SqlFunctions.toInt(new java.sql.Date(0L)), is(0)); // rounded to closest day
+ assertThat(sqlDate("1970-01-01"), is(0));
+ assertThat(sqlDate("1500-04-30"), is(unixDate("1500-04-30")));
+
+ // Gregorian shift
+ assertThat(sqlDate("1582-10-04"), is(unixDate("1582-10-04")));
+ assertThat(sqlDate("1582-10-05"), is(unixDate("1582-10-15")));
+ assertThat(sqlDate("1582-10-15"), is(unixDate("1582-10-15")));
+
+ // Test date range 0001-01-01 to 9999-12-31 required by ANSI SQL
+ for (int i = 1; i <= 9999; ++i) {
+ final String str = String.format(Locale.ROOT, "%04d-01-01", i);
+ assertThat(sqlDate(str), is(unixDate(str)));
+ }
+ }
+
+ @Test void testSqlTimeToUnixTime() {
+ assertThat(sqlTime("00:00:00"), is(unixTime("00:00:00")));
+ assertThat(sqlTime("23:59:59"), is(unixTime("23:59:59")));
+ }
+
+ @Test void testSqlTimestampToUnixTimestamp() {
+ assertThat(sqlTimestamp("1970-01-01 00:00:00"), is(0L));
+ assertThat(sqlTimestamp("2014-09-30 15:28:27.356"),
+ is(unixTimestamp("2014-09-30 15:28:27.356")));
+ assertThat(sqlTimestamp("1500-04-30 12:00:00.123"),
+ is(unixTimestamp("1500-04-30 12:00:00.123")));
+
+ // With connection time zone
+ final Timestamp epoch = java.sql.Timestamp.valueOf("1970-01-01 00:00:00");
+
+ final TimeZone est = TimeZone.getTimeZone("GMT-5:00");
+ assertThat(SqlFunctions.toLong(epoch, est),
+ is(epoch.getTime() + est.getOffset(epoch.getTime())));
+
+ final TimeZone ist = TimeZone.getTimeZone("GMT+5:00");
+ assertThat(SqlFunctions.toLong(epoch, ist),
+ is(epoch.getTime() + ist.getOffset(epoch.getTime())));
+
+ // Gregorian shift
+ assertThat(sqlTimestamp("1582-10-04 00:00:00"), is(unixTimestamp("1582-10-04 00:00:00")));
+ assertThat(sqlTimestamp("1582-10-05 00:00:00"), is(unixTimestamp("1582-10-15 00:00:00")));
+ assertThat(sqlTimestamp("1582-10-15 00:00:00"), is(unixTimestamp("1582-10-15 00:00:00")));
+
+ // Test date range 0001-01-01 to 9999-12-31 required by ANSI SQL
+ for (int i = 1; i <= 9999; ++i) {
+ final String str = String.format(Locale.ROOT, "%04d-01-01 00:00:00", i);
+ assertThat(sqlTimestamp(str), is(unixTimestamp(str)));
+ }
+ }
+
+ @Test void testUnixDateToSqlDate() {
+ assertThat(SqlFunctions.internalToDate(0), hasToString("1970-01-01"));
+ assertThat(SqlFunctions.internalToDate(unixDate("1500-04-30")),
+ hasToString("1500-04-30"));
+
+ // Gregorian shift
+ assertThat(SqlFunctions.internalToDate(unixDate("1582-10-04")),
+ hasToString("1582-10-04"));
+ assertThat(SqlFunctions.internalToDate(unixDate("1582-10-05")),
+ hasToString("1582-10-15"));
+ assertThat(SqlFunctions.internalToDate(unixDate("1582-10-15")),
+ hasToString("1582-10-15"));
+
+ // Test date range 0001-01-01 to 9999-12-31 required by ANSI SQL
+ for (int i = 1; i <= 9999; ++i) {
+ final String str = String.format(Locale.ROOT, "%04d-01-01", i);
+ assertThat(SqlFunctions.internalToDate(unixDate(str)),
+ hasToString(str));
+ }
+ }
+
+ @Test void testUnixTimeToSqlTime() {
+ assertThat(SqlFunctions.internalToTime(0), hasToString("00:00:00"));
+ assertThat(SqlFunctions.internalToTime(86399000), hasToString("23:59:59"));
+ }
+
+ @Test void testUnixTimestampToSqlTimestamp() {
+ assertThat(SqlFunctions.internalToTimestamp(0), hasToString("1970-01-01 00:00:00.0"));
+ assertThat(SqlFunctions.internalToTimestamp(unixTimestamp("2014-09-30 15:28:27.356")),
+ hasToString("2014-09-30 15:28:27.356"));
+ assertThat(SqlFunctions.internalToTimestamp(unixTimestamp("1500-04-30 12:00:00")),
+ hasToString("1500-04-30 12:00:00.0"));
+
+ // Gregorian shift
+ assertThat(SqlFunctions.internalToTimestamp(unixTimestamp("1582-10-04 00:00:00")),
+ hasToString("1582-10-04 00:00:00.0"));
+ assertThat(SqlFunctions.internalToTimestamp(unixTimestamp("1582-10-05 00:00:00")),
+ hasToString("1582-10-15 00:00:00.0"));
+ assertThat(SqlFunctions.internalToTimestamp(unixTimestamp("1582-10-15 00:00:00")),
+ hasToString("1582-10-15 00:00:00.0"));
+
+ // Test date range 0001-01-01 to 9999-12-31 required by ANSI SQL
+ for (int i = 1; i <= 9999; ++i) {
+ final String str = String.format(Locale.ROOT, "%04d-01-01 00:00:00", i);
+ assertThat(SqlFunctions.internalToTimestamp(unixTimestamp(str)), hasToString(str + ".0"));
+ }
+ }
+
+ private int sqlDate(String str) {
+ return SqlFunctions.toInt(java.sql.Date.valueOf(str));
+ }
+
+ private int sqlTime(String str) {
+ return SqlFunctions.toInt(java.sql.Time.valueOf(str));
+ }
+
+ private long sqlTimestamp(String str) {
+ return SqlFunctions.toLong(java.sql.Timestamp.valueOf(str));
+ }
+
+ private int unixDate(String str) {
+ return DateTimeUtils.dateStringToUnixDate(str);
+ }
+
+ private int unixTime(String str) {
+ return DateTimeUtils.timeStringToUnixDate(str);
+ }
+
+ private long unixTimestamp(String str) {
+ return DateTimeUtils.timestampStringToUnixDate(str);
+ }
Review Comment:
Nitpick: I can see the point in the methods above as they chain few calls, but these three are literally replacing a method call with another, maybe we can drop them and inline, wdyt?
--
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