Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: GROUP BY with timezone and small page size returns duplicate values #51258

Closed
bpintea opened this issue Jan 21, 2020 · 9 comments · Fixed by #52101
Closed

SQL: GROUP BY with timezone and small page size returns duplicate values #51258

bpintea opened this issue Jan 21, 2020 · 9 comments · Fixed by #52101
Assignees
Labels

Comments

@bpintea
Copy link
Contributor

bpintea commented Jan 21, 2020

The following query on the test employees.csv:

POST _sql
{
  "query": "SELECT YEAR(birth_date) FROM employees GROUP BY YEAR(birth_date)",
  "time_zone": "Europe/Berlin",
  "fetch_size": 1
}

will return the following paged results: null, 1952, 1952, ...
Increasing the page size to 2 will duplicate 1954.

This doesn't (easily?) reproduce without a non-Z timezone.

@elasticmachine
Copy link
Collaborator

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

@matriv
Copy link
Contributor

matriv commented Feb 7, 2020

@bpintea This "issue" has to do with not passing the "time_zone": "Europe/Belrin" param to the subsequent "cursor": "xxxxxx requests. If it's not passed the subsequent requests will use Z zoneId, thus resulting to this discrepancy in the results.

I will open a PR to clarify this in our docs.

@matriv matriv self-assigned this Feb 7, 2020
@matriv matriv removed the >bug label Feb 7, 2020
@matriv
Copy link
Contributor

matriv commented Feb 7, 2020

The query is of course executed once with the correct timezone passed with the initial HTTP request, the problem is in the HitExtractors where the timezone is set to Z for the processing of the subsequent pages.

matriv added a commit to matriv/elasticsearch that referenced this issue Feb 7, 2020
Previously, when the specified (or default) `fetchSize` led to
subsequent HTTP requests and the usage of cursors, those subsequent
cursor requests didn't use the `timezone` defined in the connection
properties. Even though the query is executed once (with the correct
timezone) the processing of the values returned by the `HitExtractors`
in the next pages was done using the default timezone `Z`. This could
lead to incorrect results.

Fix the issue by passing the initially configured timezone to each
subsequent cursor HTTP request.

Add a note in the docs to clarify that when the REST endpoint is used,
the user needs to keep passing the `time_zone` parameter to each
subsequent request, as there is no notion of a user session through in
the REST HTTP endpoint.

Relates to: elastic#51258
@bpintea
Copy link
Contributor Author

bpintea commented Feb 7, 2020

@bpintea This "issue" has to do with not passing the "time_zone": "Europe/Belrin" param to the subsequent "cursor": "xxxxxx requests. If it's not passed the subsequent requests will use Z zoneId, thus resulting to this discrepancy in the results.

Ah, I see. I haven't checked what the cursor actually stores, but:

  • the (/some?) other parameters (like page size) seem to be stored in it, since the size given to the first request is kept on subsequent pages;
  • a quick base64 decode reveals that the timezone appears in it, even if the time zone is not specified past the 1st request;
  • (I vaguely remember seeing a comment stating that only the page timeout parameter should influence the request that contains a cursor -- I'll check it on Monday).

@bpintea
Copy link
Contributor Author

bpintea commented Feb 7, 2020

The docs currently state: Do note that most parameters (outside the timeout and columnar ones) make sense only during the initial query - any follow-up pagination request only requires the cursor parameter as explained in the pagination chapter. That’s because the query has already been executed and the calls are simply about returning the found results - thus the parameters are simply ignored.

@matriv
Copy link
Contributor

matriv commented Feb 7, 2020

I opened #52080 to fix docs and also fix the issue for the JDBC driver.

@matriv
Copy link
Contributor

matriv commented Feb 8, 2020

@bpintea I was confused with the implementation. And @costin helped out (as always :) ): #52080 (comment)

The expected behaviour is not to pass the timezone in the subsequent paging requests, so there is a bug that affects both http REST endpoint and the jdbc driver.

matriv added a commit to matriv/elasticsearch that referenced this issue Feb 8, 2020
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
@matriv matriv added the >bug label Feb 8, 2020
matriv added a commit that referenced this issue Feb 11, 2020
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
matriv added a commit that referenced this issue Feb 11, 2020
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)
matriv added a commit that referenced this issue Feb 11, 2020
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)
@matriv
Copy link
Contributor

matriv commented Feb 11, 2020

master : 8f7afbd
7.x : 204d086
7.6 : 3c8ea17

@matriv
Copy link
Contributor

matriv commented Feb 11, 2020

The bug doesn't apply to 6.8 as it was introduced with #45678
So instead, only the integration tests (adapted for 6.x) were added:
4797347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants