Skip to content

SQL: Fix issue with date columns returned always in UTC #40163

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 20, 2019
Merged

SQL: Fix issue with date columns returned always in UTC #40163

merged 3 commits into from
Mar 20, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Mar 18, 2019

When selecting columns of ES type date (SQL's DATETIME) the
FieldHitExtractor was not using the timezone of the client session
but always resorted to UTC. The same behaviour (UTC only) was
encountered also for grouping keys (CompositeKeyExtractor) and
for First/Last functions on dates (TopHitsAggExtractor).

Fixes: #40152

@matriv
SQL: Fix issue with date columns returned always in UTC
f0f68e0 
When selecting columns of ES type `date` (SQL's DATETIME) the
`FieldHitExtractor` was not using the timezone of the client session
but always resorted to UTC. The same behaviour (UTC only) was
encountered also for grouping keys (`CompositeKeyExtractor`) and
for First/Last functions on dates (`TopHitsAggExtractor`).

Fixes: elastic#40152
@matriv matriv requested review from costin and astefan March 18, 2019 17:11
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv
Copy link
Contributor Author

matriv commented Mar 18, 2019

/cc @bpintea

This was referenced Mar 19, 2019
Closed
Merged
costin
costin approved these changes Mar 19, 2019
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Please raise an another issue since in the future we should improve the way we serialize/deserialize extractors. The timezone is common property yet 3 types of extractors have a reference to it and serialize/deserialize it separately.
Ideally we would find a way to save it once per cursor and reused in all these places.

@matriv
Copy link
Contributor Author

matriv commented Mar 19, 2019 •
edited
Loading

@costin thanks for reminding, Opened: #40216

astefan
astefan approved these changes Mar 20, 2019
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left two comments.

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java Outdated
sqle.getMessage());
sqle = expectThrows(SQLException.class, () -> results.getObject("test_date", Short.class));
assertEquals(format(Locale.ROOT, "Unable to convert value [%.128s] of type [DATETIME] to [Short]", of(randomDate)),
assertEquals(format(Locale.ROOT, "Unable to convert value [%.128s] of type [DATETIME] to [Short]", of(randomDate, timeZoneId)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it cumbersome to call every time this static method with the timeZoneId that's initialized outside the tests methods. Maybe create a of method inside the ResultSetTestCase class that will, in turn, call the JdbcTestUtils' of method with the timeZoneId param?

.../java/org/elasticsearch/xpack/sql/execution/search/extractor/CompositeKeyExtractorTests.java Outdated
protected CompositeKeyExtractor mutateInstance(CompositeKeyExtractor instance) throws IOException {
return new CompositeKeyExtractor(instance.key() + "mutated", instance.property(), instance.zoneId());
protected CompositeKeyExtractor mutateInstance(CompositeKeyExtractor instance) {
return new CompositeKeyExtractor(instance.key() + "mutated", instance.property(), instance.zoneId(), instance.isDateTimeBased());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this mutation mechanism accurate here? Not talking about this PR specific change, but the method in general. Shouldn't this have 4 different mutations? (the mutated key, the property, the zone id, and the freshly added boolean) Something similar to https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java#L102.

@matriv
Copy link
Contributor Author

matriv commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

@matriv
Copy link
Contributor Author

matriv commented Mar 20, 2019

@elasticmachine run
elasticsearch-ci/packaging-sample

@matriv
Copy link
Contributor Author

matriv commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

@matriv matriv merged commit 9376761 into elastic:master Mar 20, 2019
@matriv matriv deleted the mt/fix-40152 branch March 20, 2019 18:59
matriv added a commit that referenced this pull request Mar 20, 2019
@matriv
SQL: Fix issue with date columns returned always in UTC (#40163)
bc4c8e5 
When selecting columns of ES type `date` (SQL's DATETIME) the
`FieldHitExtractor` was not using the timezone of the client session
but always resorted to UTC. The same behaviour (UTC only) was
encountered also for grouping keys (`CompositeKeyExtractor`) and
for First/Last functions on dates (`TopHitsAggExtractor`).

Fixes: #40152
matriv added a commit that referenced this pull request Mar 20, 2019
@matriv
SQL: Fix issue with date columns returned always in UTC (#40163)
25f67ab 
When selecting columns of ES type `date` (SQL's DATETIME) the
`FieldHitExtractor` was not using the timezone of the client session
but always resorted to UTC. The same behaviour (UTC only) was
encountered also for grouping keys (`CompositeKeyExtractor`) and
for First/Last functions on dates (`TopHitsAggExtractor`).

Fixes: #40152
matriv added a commit that referenced this pull request Mar 20, 2019
@matriv
SQL: Fix issue with date columns returned always in UTC (#40163)
6998ea7 
When selecting columns of ES type `date` (SQL's DATETIME) the
`FieldHitExtractor` was not using the timezone of the client session
but always resorted to UTC. The same behaviour (UTC only) was
encountered also for grouping keys (`CompositeKeyExtractor`) and
for First/Last functions on dates (`TopHitsAggExtractor`).

Fixes: #40152
matriv added a commit that referenced this pull request Mar 20, 2019
@matriv
SQL: Fix issue with date columns returned always in UTC (#40163)
3a627e1 
When selecting columns of ES type `date` (SQL's DATETIME) the
`FieldHitExtractor` was not using the timezone of the client session
but always resorted to UTC. The same behaviour (UTC only) was
encountered also for grouping keys (`CompositeKeyExtractor`) and
for First/Last functions on dates (`TopHitsAggExtractor`).

Fixes: #40152
@matriv
Copy link
Contributor Author

matriv commented Mar 20, 2019

Backported to 7.x with bc4c8e5
to 7.0 with 25f67ab
to 6.7 with 6998ea7
to 6.6 with 3a627e1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin costin costin approved these changes

@astefan astefan astefan approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >bug v6.6.3 v6.7.0 v7.0.0-rc1 v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SQL: Columns of type DATETIME are returned always at UTC
6 participants
@matriv @elasticmachine @costin @astefan @jakelandis @michaelbaamonde