SQL: Fix getTime() methods in JDBC #40484

matriv · 2019-03-26T18:18:21Z

Previously, getTime(colIdx/colLabel) and
getObject(colIdx/colLabel, java.sql.Time.class) methods were computing
the time from a ZonedDateTime by applying day in millis modulo on the epoch millis
of the ZonedDateTime object. This is wrong as we need to keep the time
related fields at the timezone of the ZonedDateTime object and just
set the date info to the epoch date (01/01/1970).

Additionally fixes a testing issue as the original timezone id is converted
to an offset string when parsing the response from the server.

Previously, `getTime(colIdx/colLabel)` and `getObject(colIdx/colLabel, java.sql.Time.class)` methods were computing the time from a `ZonedDateTime` by applying day in millis modulo on the epoch millis of the `ZonedDateTime` object. This is wrong as we need to keep the time related fields at the timezone of the `ZonedDateTime` object and just set the date info to the epoch date (01/01/1970). Additionally fixes a testing issue as the original timezone id is converted to an offset string when parsing the response from the server.

elasticmachine · 2019-03-26T18:18:23Z

Pinging @elastic/es-search

matriv · 2019-03-26T18:20:12Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java

@@ -1748,4 +1753,8 @@ private Connection esWithLeniency(boolean multiValueLeniency) throws SQLExceptio
    private String asDateString(long millis) {
        return of(millis, timeZoneId);
    }
+
+    private ZoneId getZoneFromOffset(Long randomLongDate) {


I think this reveals an issue with how we send the respond from the server.
Shouldn't we keep for example Australia/Tasmania instead of +10:00`?
@costin @astefan what do you think?

+10:00 is static for any timestamp, but Australia/Tasmania offset might change (cause of test failure before).

ZoneId is better I would argue though at the same time, if this is a cause for failure it might mean that the offset rules on the server are different than that on the client which is an issue on itself.

I kind of agree that +10:00 is ambiguous, but can there be an actual issue here?
Different offset rules on server and on the client can probably happen when different Java versions are used (and offset/timezone impl. differ between the two).

The "issue" is that +10:00 is static, it doesn't include DST and it doesn't include any changes in the offset of a timezone (in our example: Australia/Tasmania) that can happen over the years for certain timezones. So if the client would further need to process the result "looses" this info from the result returned, but it still poccesses the Australia/Tasmania since this was the timezone string it set as connection/REST property.

costin

Left some comments

costin · 2019-03-27T06:34:20Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java

+        return sb.toString();
+    }
+
+    private static void rowAsString(ResultSet rs, StringBuilder sb, StringBuilder column, int columns) throws SQLException {


The method signature seems incorrect column seems to be used only locally and sb can be returned by the method - passing them as arguments doesn't offer any value.

Will fix, just used the automatic refactoring.

costin · 2019-03-27T06:36:45Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java

@@ -1748,4 +1753,8 @@ private Connection esWithLeniency(boolean multiValueLeniency) throws SQLExceptio
    private String asDateString(long millis) {
        return of(millis, timeZoneId);
    }
+
+    private ZoneId getZoneFromOffset(Long randomLongDate) {


ZoneId is better I would argue though at the same time, if this is a cause for failure it might mean that the offset rules on the server are different than that on the client which is an issue on itself.

costin · 2019-03-27T06:37:03Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java

        doWithQuery(SELECT_ALL_FIELDS, (results) -> {
            results.next();

-            java.sql.Time expectedTime = new java.sql.Time(randomLongDate % 86400000L);
+            ZoneId zoneId = getZoneFromOffset(randomLongDate);
+            java.sql.Time expectedTime = new java.sql.Time(


Shouldn't this be extracted in a utils class?

Didn't do it as it's used only once here, but I can extract it.

astefan

LGTM

astefan · 2019-03-27T07:36:51Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java

@@ -1748,4 +1753,8 @@ private Connection esWithLeniency(boolean multiValueLeniency) throws SQLExceptio
    private String asDateString(long millis) {
        return of(millis, timeZoneId);
    }
+
+    private ZoneId getZoneFromOffset(Long randomLongDate) {


I kind of agree that +10:00 is ambiguous, but can there be an actual issue here?
Different offset rules on server and on the client can probably happen when different Java versions are used (and offset/timezone impl. differ between the two).

costin

LGTM

Previously, `getTime(colIdx/colLabel)` and `getObject(colIdx/colLabel, java.sql.Time.class)` methods were computing the time from a `ZonedDateTime` by applying day in millis modulo on the epoch millis of the `ZonedDateTime` object. This is wrong as we need to keep the time related fields at the timezone of the `ZonedDateTime` object and just set the date info to the epoch date (01/01/1970). Additionally fixes a testing issue as the original timezone id is converted to an offset string when parsing the response from the server.

matriv · 2019-03-27T17:53:18Z

Backported to 7.x. with 8e049c5
to 7.0 with e39e40b
to 6.7 with e4d41d3
to 6.6 with f6072e5

matriv added >bug v7.0.0 :Analytics/SQL SQL querying v8.0.0 v7.2.0 v6.6.3 v6.7.1 labels Mar 26, 2019

matriv requested review from costin and astefan March 26, 2019 18:18

matriv commented Mar 26, 2019

View reviewed changes

Remove @repeat

60ca0b4

costin reviewed Mar 27, 2019

View reviewed changes

astefan approved these changes Mar 27, 2019

View reviewed changes

Address comments

6021f09

matriv requested a review from costin March 27, 2019 09:35

costin approved these changes Mar 27, 2019

View reviewed changes

matriv merged commit 2221cc7 into elastic:master Mar 27, 2019

matriv deleted the mt/fix-jdbc-time branch March 27, 2019 16:13

jakelandis added v7.0.0-rc2 and removed v7.0.0 labels Apr 3, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

SQL: Fix getTime() methods in JDBC #40484

SQL: Fix getTime() methods in JDBC #40484

Uh oh!

Conversation

matriv commented Mar 26, 2019

Uh oh!

elasticmachine commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

matriv commented Mar 27, 2019

Uh oh!

Uh oh!