SQL: Respect custom time zone from client in CAST #76638

Luegg · 2021-08-18T08:33:22Z

Resolves #40692

Makes sure that CASTs use the time zone specified in the according REST/JDBC options to convert strings to datetime if no timezone is specified in the string itself. Other conversions to datetime (e.g. from long) still use UTC.

This change breaks some queries using conversions from string to datetime if the string does not include a timezone. E.g. the following query will retrieve various amount of records depending on the timezone specified in the connection: SELECT hire_date FROM test_emp WHERE hire_date >= '1997-05-19T01:00:00.000'::DATETIME

The following queries are not affected:

SELECT hire_date FROM test_emp WHERE hire_date >= '1997-05-19T01:00:00.000' // timezone is applied in the comparison
SELECT hire_date FROM test_emp WHERE hire_date >= '1997-05-19T01:00:00.000Z'::DATETIME // timezone specified in the string
SELECT hire_date FROM test_emp WHERE hire_date = '1997-05-19||/y' // timezone is applied in the comparison
SELECT hire_date FROM test_emp WHERE hire_date >= 123456789::DATETIME // still uses UTC

elasticmachine · 2021-08-18T14:39:45Z

Pinging @elastic/es-ql (Team:QL)

Luegg · 2021-08-18T14:44:54Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/Converter.java

+     * Converts `value` to the target type considering `zoneId` if needed.
+     * `zoneId` can be null and it is ignored for all non-timezone aware conversions.
+     */
+    Object convert(Object value, ZoneId zoneId);


I've also investigated whether the zoneId could be stored in the converter but this would be a lot more invasive change.

Indeed, adding a specific method for one specific data type doesn't feel ideal, but still better than instantiating the converter per session.

Resolves elastic#40692

bpintea

Lg, only left minor observations.

bpintea · 2021-08-19T09:56:15Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DateUtils.java

+     * Parses the given string into a DateTime using `defaultZone` as a default timezone.
     */
-    public static ZonedDateTime asDateTime(String dateFormat) {
+    public static ZonedDateTime asDateTime(String dateFormat, ZoneId defaultZone) {


Why keep the "default" in defaultZone? This would be the user-specified TZ (or null/default), right?

bpintea · 2021-08-19T10:00:53Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

@@ -1173,52 +1170,6 @@ protected LogicalPlan rule(OrderBy ob) {
        }
    }

-    private static class ImplicitCasting extends AnalyzerRule<LogicalPlan> {


There's a commented instantiation of this rule (L124). Not sure why this was kept, but I think commented/dead code isn't typically kept around, so maybe remove that line too.

👍 I didn't thin about searching for the class. Thanks!

bpintea · 2021-08-19T10:08:57Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/Converter.java

+     * Converts `value` to the target type considering `zoneId` if needed.
+     * `zoneId` can be null and it is ignored for all non-timezone aware conversions.
+     */
+    Object convert(Object value, ZoneId zoneId);


Indeed, adding a specific method for one specific data type doesn't feel ideal, but still better than instantiating the converter per session.

bpintea · 2021-08-19T10:09:59Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/SqlDataTypeConverterTests.java

@@ -52,6 +54,8 @@

 public class SqlDataTypeConverterTests extends ESTestCase {

+    private static final ZoneId MINUS_5 = ZoneId.of("-05:00");


nit:

Suggested change

private static final ZoneId MINUS_5 = ZoneId.of("-05:00");

private static final ZoneId MINUS_5H = ZoneId.of("-05:00");

bpintea · 2021-08-19T10:27:10Z

x-pack/plugin/sql/qa/server/src/main/resources/datetime-custom-tz.csv-spec

+;
+
+castStringWithoutTZToDatetimeInScript-TZ[Etc/GMT-1]
+SELECT hire_date::string,


Supernit: the rest of the tests use capitalised ::STRING, but I was actually wondering if the conversion is [always] necessary?

👍 for the capitalization.

But yes, as soon as you get timestamps in another timezone as UTC, the JDBC issue mentioned in #40779 surfaces.

bpintea · 2021-08-19T10:39:09Z

.../plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java

@@ -43,9 +43,7 @@ public CsvSpecTestCase(String fileName, String groupName, String testName, Integ

    @Override
    protected final void doTest() throws Throwable {
-        // Run the time tests always in UTC
-        // TODO: https://github.com/elastic/elasticsearch/issues/40779


If we don't (yet) randomise the timezones for the QA tests ref'd in this comment (which I think is the case), maybe we should keep the comment. (BTW, the file ref'd in the issue contains a similar comment.)

Maybe I got this wrong but when the comment was added, time.csv-spec has been special cased (see https://github.com/elastic/elasticsearch/pull/40776/files#diff-387c1ea94dd5d111032f384770a8244f7d4942ac1fd91a68b397614212d59f04R44). But the special case has been removed in the meantime and I think the comment is no longer accurate. Or the reality is now more complicated as described in https://github.com/elastic/elasticsearch/pull/76638/files#diff-aceee053cb68e1bb219cfe4256635c7f77d86b46eb943e5ad42bc7bb360ca945R125

bpintea · 2021-08-19T10:39:44Z

x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/type/DataTypeConversionTests.java

@@ -33,6 +35,8 @@

 public class DataTypeConversionTests extends ESTestCase {

+    public static final ZoneId MINUS_5 = ZoneId.of("-05:00");


suggestion: MINUS_5H.

Thx, that's nicer indeed 👍

Luegg · 2021-08-19T12:53:08Z

...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java

-        return new ScriptTemplate(
-                formatTemplate(format("{sql}.", "cast({},{})", fieldAsScript.template())),
+
+        if (DataTypes.isDateTime(dataType)) {


I'm not entirely sure what's better here: Only passing the timezone for datetime conversions results in a smaller diff of the changes and the resulting scripts are also not unnecessarily large. But it breaks a bit with consistency and might become a problem when more conversions start depending on the timezone.

Not sure whether anyone has a stronger opinion on this?

astefan

Looks good in general, but to me it's unclear what exactly the original bug report covers and what this PR is addressing.
For example, what happens with CASTs applied on fields. All examples and tests I see refer to date literals only. Also, any more complex queries (a group by on a CASTed string) - SELECT CAST(CAST(birth_date AS DATETIME) AS STRING) t FROM test_emp WHERE YEAR(birth_date)=2021 GROUP BY t - are not covered.

astefan · 2021-08-19T15:04:15Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DateUtils.java

@@ -51,10 +52,16 @@ public static ZonedDateTime asDateTime(long millis) {
        return ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), UTC);


This method can simply call the new asDateTime method below.

astefan · 2021-08-19T20:45:01Z

.../sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/CastProcessor.java

    }

    Converter converter() {
        return conversion;
    }

+    ZoneId zoneId() { return zoneId; }


Please, follow the style of the other methods: a multi-line method, rather than a single-liner.

Luegg · 2021-09-08T08:51:18Z

Note to myself: Consider reusing DATETIME_PARSE (https://www.elastic.co/guide/en/elasticsearch/reference/master/sql-functions-datetime.html#sql-functions-datetime-datetimeparse) to avoid bwc issues.

Merge origin/master into fix/defaultTimezoneInCasts

a7cb38c

elasticsearchmachine added the v8.0.0 label Aug 18, 2021

Luegg force-pushed the fix/defaultTimezoneInCasts branch 4 times, most recently from da174cd to 1741835 Compare August 18, 2021 14:23

Luegg added v7.14.1 v7.15.0 :Analytics/SQL SQL querying >bug labels Aug 18, 2021

Luegg requested review from costin, astefan and bpintea August 18, 2021 14:39

Luegg marked this pull request as ready for review August 18, 2021 14:39

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Aug 18, 2021

Luegg commented Aug 18, 2021

View reviewed changes

Luegg added 2 commits August 18, 2021 16:56

Merge origin/master into fix/defaultTimezoneInCasts

d85f627

SQL: Consider custom time zone from client in CAST

6f001ce

Resolves elastic#40692

Luegg force-pushed the fix/defaultTimezoneInCasts branch from 1741835 to 6f001ce Compare August 18, 2021 14:57

bpintea approved these changes Aug 19, 2021

View reviewed changes

Luegg added 2 commits August 19, 2021 14:45

address comments

43f991a

Merge origin/master into fix/defaultTimezoneInCasts

26dea63

Luegg commented Aug 19, 2021

View reviewed changes

mark-vieira added v7.16.0 and removed v7.15.0 labels Aug 19, 2021

astefan reviewed Aug 20, 2021

View reviewed changes

Luegg added 2 commits August 25, 2021 10:24

Merge origin/main into fix/defaultTimezoneInCasts

a9d9109

address comments

1d03cb0

Luegg force-pushed the fix/defaultTimezoneInCasts branch from f348f35 to 1d03cb0 Compare August 25, 2021 08:26

martijnvg added v7.14.2 and removed v7.14.1 labels Aug 26, 2021

Luegg marked this pull request as draft August 30, 2021 11:21

henningandersen added v7.14.3 and removed v7.14.2 labels Sep 15, 2021

danhermann added v8.1.0 and removed v7.16.0 labels Oct 27, 2021

arteam removed the v8.0.0 label Jan 12, 2022

Luegg closed this Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Respect custom time zone from client in CAST #76638

SQL: Respect custom time zone from client in CAST #76638

Luegg commented Aug 18, 2021 •

edited

Loading

elasticmachine commented Aug 18, 2021

Luegg Aug 18, 2021

bpintea Aug 19, 2021

bpintea left a comment

bpintea Aug 19, 2021

bpintea Aug 19, 2021

Luegg Aug 19, 2021

bpintea Aug 19, 2021

bpintea Aug 19, 2021

bpintea Aug 19, 2021

Luegg Aug 19, 2021

bpintea Aug 19, 2021

Luegg Aug 19, 2021

bpintea Aug 19, 2021

Luegg Aug 19, 2021 •

edited

Loading

Luegg Aug 19, 2021

astefan left a comment

astefan Aug 19, 2021

astefan Aug 19, 2021

Luegg commented Sep 8, 2021

		@@ -52,6 +54,8 @@

		public class SqlDataTypeConverterTests extends ESTestCase {

		private static final ZoneId MINUS_5 = ZoneId.of("-05:00");

	private static final ZoneId MINUS_5 = ZoneId.of("-05:00");
	private static final ZoneId MINUS_5H = ZoneId.of("-05:00");

		@@ -33,6 +35,8 @@

		public class DataTypeConversionTests extends ESTestCase {

		public static final ZoneId MINUS_5 = ZoneId.of("-05:00");

		@@ -51,10 +52,16 @@ public static ZonedDateTime asDateTime(long millis) {
		return ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), UTC);

SQL: Respect custom time zone from client in CAST #76638

SQL: Respect custom time zone from client in CAST #76638

Conversation

Luegg commented Aug 18, 2021 • edited Loading

elasticmachine commented Aug 18, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luegg Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Luegg commented Sep 8, 2021

Luegg commented Aug 18, 2021 •

edited

Loading

Luegg Aug 19, 2021 •

edited

Loading