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: Fix issue with timezone for JDBC pagination #52080

Closed
wants to merge 5 commits into from

Conversation

matriv
Copy link
Contributor

@matriv matriv commented 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: #51258

@elasticmachine
Copy link
Collaborator

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

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
@matriv matriv force-pushed the docs-test-paging-timezone branch from 4bc0ef8 to c7f9719 Compare February 7, 2020 20:51
@matriv
Copy link
Contributor Author

matriv commented Feb 8, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-matrix-windows

@costin
Copy link
Member

costin commented Feb 8, 2020

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.

That's a bug - the HitExtractors are serialized in the cursor using the initial timezone - it's a bug to not use that timezone which is available from the stream (SqlStreamInput).
Sending any timezone in a pagination request is a bug - it can either be redundant (we already know about it) or different (what do we do then); we should check that and throw an exception if that happens.
We already have all the information we need, the onus is on us to preserve it around.

@matriv
Copy link
Contributor Author

matriv commented Feb 8, 2020

Closing this as the approach was incorrect.
Thanks @costin for helping out!
Opened #52101 instead, which fixes the bug, and no doc update is required.

@matriv matriv closed this Feb 8, 2020
@matriv matriv deleted the docs-test-paging-timezone branch February 13, 2020 20:07
@ywelsch
Copy link
Contributor

ywelsch commented Feb 28, 2020

Please remove version labels when closing a PR, as these are used to drive release notes

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 this pull request may close these issues.

4 participants