SQL: Fix issue with timezone when paginating #52101

matriv · 2020-02-08T23:09:07Z

Previously, when the specified (or default) fetchSize led to
subsequent HTTP requests and the usage of cursors, those subsequent
were no longer using the client timezone specified in the initial
SQL query. As a consequence, Even though the query is executed once
(with the correct timezone) the processing of the query results by
the HitExtractors in the next pages was done using the default
timezone Z. This could lead to incorrect results.

Fix the issue by correctly using the initially specified timezone,
which is found in the deserialisation of the cursor string.

Fixes: #51258

Previously, when the specified (or default) fetchSize led to subsequent HTTP requests and the usage of cursors, those subsequent were no longer using the client timezone specified in the initial SQL query. As a consequence, Even though the query is executed once (with the correct timezone) the processing of the query results by the HitExtractors in the next pages was done using the default timezone `Z`. This could lead to incorrect results. Fix the issue by correctly using the initially specified timezone, which is found in the deserialisation of the cursor string. Fixes: elastic#51258

elasticmachine · 2020-02-08T23:09:09Z

Pinging @elastic/es-search (:Search/SQL)

astefan

LGTM

astefan · 2020-02-09T15:50:39Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

+            Map<String, Object> expected = new HashMap<>();
+            if (i == 0) {
+                expected.put("columns", singletonList(
+                        columnInfo(mode, "tz", "integer", JDBCType.INTEGER, 11)));
+            }


Can't you move this part in the if (i==0) {} else {} above?

astefan · 2020-02-09T16:43:55Z

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

+        Properties connectionProperties = connectionProperties();
+        connectionProperties.put(JDBC_TIMEZONE, zoneId.toString());
+        try (Connection c = esJdbc(connectionProperties);
+             Statement s = c.createStatement()) {


Can you move this line on the above one?

Sure, just kept the style from the other tests, e.g.: https://github.com/elastic/elasticsearch/pull/52101/files/a488512e3abd83b8de95ef0cbfcd9b2adc2b8a86#diff-7eb5a40bcec78e0f582b7ef886430b28R95

A(n appropriate) formatter should help in these style-adjusting cases.

bpintea · 2020-02-10T09:14:19Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

+                "{\"query\":\"SELECT DATE_PART('TZOFFSET', date) AS tz FROM test_date_timezone ORDER BY date\","
+                        + "\"time_zone\":\"" + zoneId.getId() + "\", "
+                        + "\"mode\":\"" + mode + "\", "
+                        + "\"fetch_size\":2}";


I think it's perfectly fine as is, but was curious about the reason - if any - for choosing a fetch_size of 2, vs. 1, which would simplify the test just a bit.

Fetch size 2 and odd number of rows ('5') makes the last page to return 1 row.

Sure, there'll be pages with 1 and 2 rows as the j-based for makes it obvious.
But I was only curious to understand why is that desired in respect to what the test checks (i.e. all rows have the same timezone offset). Sorry if missing anything obvious. :-)

Doesn't have to do with the bug fixed, just another safety testing that the fetch size behavior is correct.

bpintea

LGTM

costin

LGTM

costin · 2020-02-10T13:58:27Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java

@@ -80,13 +83,14 @@ public static void operation(PlanExecutor planExecutor, SqlQueryRequest request,
            planExecutor.sql(cfg, request.query(), request.params(),
                    wrap(p -> listener.onResponse(createResponseWithSchema(request, p)), listener::onFailure));
        } else {
-            planExecutor.nextPage(cfg, Cursors.decodeFromString(request.cursor()),
-                    wrap(p -> listener.onResponse(createResponse(request, null, p)),
+            Tuple<Cursor, ZoneId> decoded = Cursors.decodeFromStringWithZone(request.cursor());


Is Cursors.decodeFromString used anywhere else ? It might be that decoding a tuple of timezone or adding it as a property in the Cursor class should be the default.

I'm planning to do that in an upcoming PR.

Ad discussed, will not pass the zoneId into the Cursor, as zoneId is the responsibility of SqlInput/OutputStreams to handle. Instead, I remove the decodeFromString and only use the decodeFromStringWithZone, so it's obvious for consumers that the zoneId is always decoded as well and returned as part of the returned Tuple.

costin · 2020-02-10T13:59:16Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java

@@ -68,7 +71,7 @@ protected void doExecute(Task task, SqlQueryRequest request, ActionListener<SqlQ
    /**
     * Actual implementation of the action. Statically available to support embedded mode.
     */
-    public static void operation(PlanExecutor planExecutor, SqlQueryRequest request, ActionListener<SqlQueryResponse> listener,
+    private static void operation(PlanExecutor planExecutor, SqlQueryRequest request, ActionListener<SqlQueryResponse> listener,


Nit: why make it private ? I think this will make things a bit harded in debug project.

Ah, didn't think of that, Thx, I'll revert

Previously, when the specified (or default) fetchSize led to subsequent HTTP requests and the usage of cursors, those subsequent were no longer using the client timezone specified in the initial SQL query. As a consequence, Even though the query is executed once (with the correct timezone) the processing of the query results by the HitExtractors in the next pages was done using the default timezone Z. This could lead to incorrect results. Fix the issue by correctly using the initially specified timezone, which is found in the deserialisation of the cursor string. Fixes: #51258 (cherry picked from commit 8f7afbd)

Cherry pick and adapt tests to validate correct behaviour regarding processing of results that involve the use of the client configured timezone by the HitExtractors when paginating over the results of the query (use of cursors). (cherry picked from commit 8f7afbd)

matriv added >bug :Analytics/SQL SQL querying v8.0.0 v6.8.7 v7.7.0 v7.6.1 labels Feb 8, 2020

matriv requested review from costin, astefan and bpintea February 8, 2020 23:09

matriv mentioned this pull request Feb 8, 2020

SQL: Fix issue with timezone for JDBC pagination #52080

Closed

astefan approved these changes Feb 9, 2020

View reviewed changes

Address comments

c891567

bpintea reviewed Feb 10, 2020

View reviewed changes

bpintea approved these changes Feb 10, 2020

View reviewed changes

costin approved these changes Feb 10, 2020

View reviewed changes

matriv and others added 3 commits February 10, 2020 19:21

revert private on operation()

81605e8

Merge remote-tracking branch 'upstream/master' into fix-51258

0120357

make sure decodefromstringWithZone is always used

e95628a

matriv merged commit 8f7afbd into elastic:master Feb 11, 2020

matriv deleted the fix-51258 branch February 11, 2020 13:59

matriv added the backport pending label Feb 11, 2020

matriv removed backport pending v6.8.7 labels Feb 11, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Fix issue with timezone when paginating #52101

SQL: Fix issue with timezone when paginating #52101

matriv commented Feb 8, 2020

elasticmachine commented Feb 8, 2020

astefan left a comment

astefan Feb 9, 2020

astefan Feb 9, 2020

matriv Feb 9, 2020

bpintea Feb 10, 2020

bpintea Feb 10, 2020

matriv Feb 10, 2020

bpintea Feb 10, 2020 •

edited

Loading

matriv Feb 10, 2020

bpintea left a comment

costin left a comment

costin Feb 10, 2020

matriv Feb 10, 2020

matriv Feb 11, 2020

costin Feb 10, 2020

matriv Feb 10, 2020

SQL: Fix issue with timezone when paginating #52101

SQL: Fix issue with timezone when paginating #52101

Conversation

matriv commented Feb 8, 2020

elasticmachine commented Feb 8, 2020

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea Feb 10, 2020 •

edited

Loading