SQL: Introduce SQL TIME data type #39802

matriv · 2019-03-07T17:59:08Z

Support ANSI SQL's TIME type by introducing a runtime-only
ES SQL time type.

Closes: #38174

elasticmachine · 2019-03-07T17:59:09Z

Pinging @elastic/es-search

Support ANSI SQL's TIME type by introductin a runtime-only ES SQL time type. Closes: elastic#38174

costin

Thanks for working on this.
I've left a first round of comments.

costin · 2019-03-11T11:40:22Z

x-pack/plugin/sql/qa/src/main/resources/time.csv-spec

+1955-01-21 00:00:00Z | Maliniak
+;
+
+timeAsFilter_NoMatch


Please add a test with an ORDER BY on a time (datetime cast).

costin · 2019-03-11T11:56:43Z

...va/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java

    protected abstract Object doFold(ZonedDateTime dateTime);
-
+
+    protected Object doFold(OffsetTime time) {


If the default implementation is to throw an exception in case of a time object, then the folding should not occur in the first place.
In other words, the subclasses that support time should extend the existing methods instead of the opposite, all classes need to override the default implementation which throws an exception about "time".

costin · 2019-03-11T12:02:47Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeProcessor.java

+            return doProcess(((ZonedDateTime) input).withZoneSameInstant(zoneId));
+        }
+        if (input instanceof OffsetTime) {
+            return doProcess(((OffsetTime) input).withOffsetSameInstant(ZoneOffset.UTC));


Why should the time be converted to UTC? The zoneId of the function should be used instead (potentially by converting it to a ZoneOffset or going through an Instant before).

That's wrong, fixed and added tests.

yep, that's wrong

costin · 2019-03-11T12:03:18Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeProcessor.java

    abstract Object doProcess(ZonedDateTime dateTime);
-}
+
+    Object doProcess(OffsetTime time) {


Same as above - why go through the hassle of creating a time object if only to discard it in the doProcess.

costin · 2019-03-11T12:11:35Z

...n/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java

    }

+    public static Integer dateTimeChrono(OffsetTime time, String tzId, String chronoName) {
+        return dateTimeChrono(time.withOffsetSameInstant(ZoneId.of(tzId).getRules().getOffset(Instant.now())),


Since a ZoneId can be ZoneOffset, the conversion process should be promoted to a static method in DateUtils.

costin · 2019-03-11T12:36:04Z

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

    public static final String EMPTY = "";

-    private static final DateTimeFormatter ISO_WITH_MILLIS = new DateTimeFormatterBuilder()
+    private static final DateTimeFormatter ISO_DATE_WITH_MILLIS = new DateTimeFormatterBuilder()


Doesn't the JDBC driver have access to StringUtils proto? Meaning these formatters would not be needed anymore...

Good catch, removing and using StringUtils.

costin · 2019-03-11T12:40:15Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Arithmetics.java

-        }
-
-        return l.plus(r);
+    static Temporal add(Temporal l, Period r) {


Why delegate to periodArithmetics instead of having its content here?
Same applies to the other add and sub methods.

Leftover of some intermediate state. will change.

Ah, sorry no, this is because the periodArithmetics/durationArithmetics are also used for the minus.

Temporal::plus/minus actually delegate to TemporalAmount::addTo/substractFrom which is what Duration and Period are. Considering the latter types are not used anymore, it makes sense to have just one method with Temporal and TemporalAmount and a boolean or enum to indicate whether it's addition or substraction.

It's easier and more accurate vs the closure which is too broad. I would still opt for type checks on either side to make sure no unknown objects are being sent.

The type checks have been added to BinaryArithmeticOperation

But how can I have one method since There is some difference on how Duration & Period are handled, I wouldn't like to make instance of on the TemporalAmount.

costin · 2019-03-11T12:41:34Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Arithmetics.java

+        }
+
+        if (l instanceof OffsetTime) {
+            r = r.withDays(0).withMonths(0).withYears(0);


This creates 2 temporary objects - use of or Instant instead.

This is a Period, couldn't find an alternative way.

Period contains years/months/days - if everything is zero then Period.ZERO should be returned.
Otherwise Period.of(int, int, int) works.

I was confused that Period can also have hours,mins, etc..
Thx and apologies for that.

costin · 2019-03-11T12:43:43Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Arithmetics.java

-        }
-
-        return l.plus(r);
+    static Temporal add(Temporal l, Period r) {


Using Temporal is too broad without validation is dangerous. We actually accept only ZonedDateTime and OffsetTime. Ideally we should check this up to the source however this creates too many methods - yet at some point we need to do the "ugly" part of checking the actual types and throwing an exception otherwise.

costin · 2019-03-11T12:44:48Z

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

                return DataType.DOUBLE;
            case "date":
                return DataType.DATE;
+            case "time":


I find time after datetime/timestamp" more intuitive.

astefan

Left few comments.
Also, you should add tests to SqlProtocolTestCase.

astefan · 2019-03-11T11:52:43Z

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

        .appendOffsetId()
        .toFormatter(Locale.ROOT);

+    static final DateTimeFormatter ISO_WITH_MILLIS = new DateTimeFormatterBuilder()


Since the other one is called ISO_DATE_WITH_MILLIS, wouldn't this be better off as ISO_TIME_WITH_MILLIS?

astefan · 2019-03-11T11:57:54Z

x-pack/plugin/sql/qa/src/main/resources/time.csv-spec

+FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;
+
+ d:i     | m:i      | h:i
+0        |0         |0


TIME tests with data from test_emp are kind of half useful. Should we consider updating our test data?

Yep, will do it in an upcoming PR if you don't mind.

For the moment I added tests here: https://github.com/elastic/elasticsearch/pull/39802/files#diff-a8ce71426601af8d094e40cafc9a4418

astefan · 2019-03-11T11:58:30Z

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

            .appendOffsetId()
            .toFormatter(Locale.ROOT);

+    private static final DateTimeFormatter ISO_WITH_MILLIS = new DateTimeFormatterBuilder()


Maybe ISO_TIME_WITH_MILLIS?

astefan · 2019-03-11T12:02:40Z

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


-    public static TypeResolution isNumericOrDate(Expression e, String operationName, ParamOrdinal paramOrd) {
-        return isType(e, dt -> dt.isNumeric() || dt.isDateBased(), operationName, paramOrd, "date", "datetime", "numeric");
+    public static TypeResolution isDateOrTime(Expression e, String operationName, ParamOrdinal paramOrd) {


isTemporal sounds better?

astefan · 2019-03-11T12:03:00Z

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

+        return isType(e, dt -> dt.isDateBased() || dt.isTimeBased(), operationName, paramOrd, "date", "time", "datetime");
+    }
+
+    public static TypeResolution isNumericOrDateOrTime(Expression e, String operationName, ParamOrdinal paramOrd) {


isNumericOrTemporal sounds better?

Imho it's more clear like it is now.

astefan · 2019-03-11T12:27:25Z

.../src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java

    @Override
    protected TypeResolution resolveWithIntervals() {
-        if (right().dataType().isDateBased() && DataTypes.isInterval(left().dataType())) {
+        if ((right().dataType().isDateBased() || right().dataType().isTimeBased()) && DataTypes.isInterval(left().dataType())) {


There are many calls for dataType().isDateBased() || dataType().isTimeBased(). I think it's worth adding to DataType a method that should do this check. Maybe called isTemporalBased?

astefan · 2019-03-11T12:42:24Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/EscapedFunctionsTests.java

                ex.getMessage());
    }

    public void testTimeLiteralUnsupported() {


This method shouldn't be called like this anymore.

astefan · 2019-03-11T12:52:02Z

...n/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java

    }

-    private static Integer dateTimeChrono(ZonedDateTime dateTime, ChronoField field) {
+    private static Integer dateTimeChrono(Temporal dateTime, ChronoField field) {


Isn't Temporal too broad here?

It's an internal method, called only with ZonedDateTime or OffsetTime so it's not dangerous and avoids duplication.

matriv · 2019-03-28T01:31:02Z

@costin @astefan
Thank you very much for the comments.
Since the PR is already pretty big I will followup with 2 more:

A PR that changes the data set to add some times (currently birth_date & hire_date are always 00:00:00.000 and adds more integ tests
A PR that changes the order of code blocks to DATETIME, DATE, TIME in order of priority

matriv · 2019-03-28T09:35:09Z

@elasticmachine run elasticsearch-ci/2

astefan

Looking good. Left two comments.

astefan · 2019-03-28T12:10:43Z

.../org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessorTests.java

+    protected DateTimeProcessor mutateInstance(DateTimeProcessor instance) {
        DateTimeExtractor replaced = randomValueOtherThan(instance.extractor(), () -> randomFrom(DateTimeExtractor.values()));
-        return new DateTimeProcessor(replaced, UTC);
+        return new DateTimeProcessor(replaced, randomValueOtherThan(UTC, ESTestCase::randomZone));


Why do you use here random other than UTC?

No reason, I will change.

astefan · 2019-03-28T12:25:21Z

.../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeFunction.java

+    }
+
+    @Override
+    public Object fold() {


Completely overriding this method here doesn't feel right. The field folding and the ZonedDateTime result is already handled in BaseDateTimeFunction...

Could this code be integrated in/moved to the BaseDateTimeFunction class and the concrete classes to implement only a doFold method? Maybe using a generic parameter for doFold depending on the class that implements it to be a date-type of class or a time-type of class?

Object doFold(T dateOrTime)

I decided to do the refactoring mentioned here: #40566 and just delegate to the processor.

costin

Left another round of comments.
Since OffsetTime is introduced in scripting, there should be some tests with HAVING against some kind of time component to make sure nothing wack happens.

costin · 2019-03-28T14:50:19Z

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


    private JdbcDateUtils() {}

+    // Not available in Java 8


Then why use it?

In > Java8 there is already a LocalDate.EPOCH constant, but since we're 8 compatible it cannot be used.

costin · 2019-03-28T14:53:31Z

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

                return floatValue(v); // Float might be represented as string for infinity and NaN values
            case DATE:
                return JdbcDateUtils.asDateTimeField(v, JdbcDateUtils::asDate, Date::new);
+            case TIME:


You could use static imports for JdbcDateUtils.asXXX methods like in the JdbcResultSet

costin · 2019-03-28T14:54:16Z

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

-        List<URL> urls = JdbcTestUtils.classpathResources("/*.csv-spec");
-        assertTrue("Not enough specs found " + urls.toString(), urls.size() > 15);
-        return readScriptSpec(urls, specParser());
+        Parser parser = specParser();


Why remove the classpath scanning?

thx for catching it, I messed up the merge...

costin · 2019-03-28T14:55:46Z

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

        }
-
+
+        if (value instanceof OffsetTime) {


I keep finding the insertion order inconsistent - is it alphabetical or usage based?

Changing it on usage based, but will come up with a followup PR to change it everywhere.

costin · 2019-03-28T14:58:23Z

...c/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/MinuteOfDay.java

        return MinuteOfDay::new;
    }

+    @Override


Push the method in the parent class to avoid having to repeat it in its children.

the method was added to TimeFunction but forgot to remove it from the children, thx!

costin · 2019-03-28T15:00:17Z

...main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeProcessor.java

+        if (input instanceof OffsetTime) {
+            return doProcess(asTimeAtZone((OffsetTime) input, zoneId()));
+        }
+        if (input instanceof ZonedDateTime) {


This section could be delegated to the super class which already does ZonedDateTime handling.

costin · 2019-03-28T15:22:52Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Arithmetics.java

+        }
+
+        if (l instanceof OffsetTime) {
+            r = r.withDays(0).withMonths(0).withYears(0);


Period contains years/months/days - if everything is zero then Period.ZERO should be returned.
Otherwise Period.of(int, int, int) works.

costin · 2019-03-28T15:26:40Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Arithmetics.java

-        }
-
-        return l.plus(r);
+    static Temporal add(Temporal l, Period r) {


Temporal::plus/minus actually delegate to TemporalAmount::addTo/substractFrom which is what Duration and Period are. Considering the latter types are not used anymore, it makes sense to have just one method with Temporal and TemporalAmount and a boolean or enum to indicate whether it's addition or substraction.

It's easier and more accurate vs the closure which is too broad. I would still opt for type checks on either side to make sure no unknown objects are being sent.

costin · 2019-03-28T15:28:26Z

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


    public static final ZoneId UTC = ZoneId.of("Z");
    public static final String DATE_PARSE_FORMAT = "epoch_millis";
+    // Not available in Java 8


Again, what's with this comment? If something is not available how can it be defined?

Changing the comment to // In Java 8 LocalDate.EPOCH is not yet available, as one might wonder why we don't use the predefined constant and will be a marker to remove it when Java8 compatibility is dropped.

matriv · 2019-03-29T16:06:06Z

@costin @astefan As discussed, grouping by TIME is currently disabled as it requires more work to implement regarding painless scripting. Tracked here: #40639

matriv · 2019-03-29T17:39:28Z

@elasticmachine run elasticsearch-ci/2

matriv · 2019-03-29T20:20:42Z

@elasticmachine run elasticsearch-ci/2

costin

LGTM

costin · 2019-03-29T21:47:10Z

x-pack/plugin/sql/qa/src/main/resources/time.csv-spec


+
+timeAsHavingFilter
+SELECT MINUTE_OF_HOUR(MAX(birth_date)::TIME + INTERVAL 10 MINUTES) as minute, gender FROM test_emp GROUP BY gender HAVING CAST(MAX(birth_date) AS TIME) = CAST('00:00:00.000' AS TIME) ORDER BY gender;


costin · 2019-03-29T21:53:11Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Arithmetics.java

            return null;
        }

        if (l instanceof OffsetTime) {


You could fold the two methods by passing the duration/period as TemporalAmount and then do a type check internally:

if (l instanceof OffsetTime) { if (r instanceof Period) { return l; } else if (r instanceof Duration) { r = ((Duration) r).ofMillis(..); } else { throw new SqlIllegalArgumentException("Unsupported Temporal type [{}]", r); }

it's not too ugly and avoid the null check and if (operation == ) repetition.

Imho since we know if it's a Period or Duration it's "ugly" to merge them into TemporalAmount and then do another instanceof (we do it already for the OffsetTime).

Full format for TIME is `00:00:00.123456789+10:00` so 24 chars long.

astefan

LGTM

astefan · 2019-04-01T08:53:08Z

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

    private JdbcDateUtils() {}

-    // Not available in Java 8
+    // In Java 8 LocalDate.EPOCH is not yet available


This comment sounds like EPOCH will soon be added to Java 8.

astefan · 2019-04-01T09:30:12Z

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

+        assertQuery("SELECT CAST(-26853765751000 AS DATE)", "CAST(-26853765751000 AS DATE)",
+            "date", "1119-01-15T00:00:00.000Z", 24);
+
+        assertQuery("SELECT CAST('12:29:25.123Z' AS TIME)", "CAST('12:29:25.123Z' AS TIME)",


The tests' values don't reflect the maximum precision of the TIME type.

As discussed we don't currently return more than 3 digits (millis), so changed it to 18.

Currently we only display down to millis.

matriv · 2019-04-01T21:30:14Z

@costin @astefan Many thx for the repeated reviews!

Support ANSI SQL's TIME type by introductin a runtime-only ES SQL time type. Closes: #38174 (cherry picked from commit 046ccd4)

Support ANSI SQL's TIME type by introductin a runtime-only ES SQL time type. Closes: elastic#38174

matriv added :Analytics/SQL SQL querying v8.0.0 v7.2.0 labels Mar 7, 2019

matriv requested review from astefan and costin March 7, 2019 17:59

SQL: Introduce SQL TIME data type

9aec487

Support ANSI SQL's TIME type by introductin a runtime-only ES SQL time type. Closes: elastic#38174

matriv force-pushed the mt/impl-38174 branch from a81126a to 9aec487 Compare March 7, 2019 19:46

costin reviewed Mar 11, 2019

View reviewed changes

astefan reviewed Mar 11, 2019

View reviewed changes

matriv added 6 commits March 14, 2019 21:01

Merge remote-tracking branch 'upstream/master' into mt/impl-38174

dc0b3a2

Merge remote-tracking branch 'upstream/master' into mt/impl-38174

126a4a0

Address comments pt1

1994a35

Merge remote-tracking branch 'upstream/master' into mt/impl-38174

cd4931a

Address comments pt2

7dac479

Fix Jdbc for TIME data type

d52455f

matriv requested review from astefan and costin March 28, 2019 01:28

remove unused imports

8d70d39

astefan reviewed Mar 28, 2019

View reviewed changes

costin reviewed Mar 28, 2019

View reviewed changes

matriv and others added 3 commits March 29, 2019 16:16

Address comments

1778fe0

Disable grouping by TIME

f495f47

Merge remote-tracking branch 'upstream/master' into mt/impl-38174

afaa81e

matriv requested review from astefan and costin and removed request for costin March 29, 2019 16:04

matriv added the >feature label Mar 29, 2019

matriv mentioned this pull request Mar 29, 2019

SQL: Implement CURRENT_TIME/CURTIME functions #40662

Merged

costin approved these changes Mar 29, 2019

View reviewed changes

matriv and others added 3 commits March 29, 2019 23:32

Merge remote-tracking branch 'upstream/master' into mt/impl-38174

60e923b

Fix docs

e131ba2

Increase precision for TIME

e752f68

Full format for TIME is `00:00:00.123456789+10:00` so 24 chars long.

astefan approved these changes Apr 1, 2019

View reviewed changes

astefan reviewed Apr 1, 2019

View reviewed changes

matriv added 2 commits April 1, 2019 17:53

Remove -6 digits for time precision.

5f91080

Currently we only display down to millis.

fix precision in docs

82a7068

matriv merged commit 046ccd4 into elastic:master Apr 1, 2019

matriv deleted the mt/impl-38174 branch April 1, 2019 21:30

matriv mentioned this pull request Apr 1, 2019

SQL: Refactor fold() Date/TimeFunctions #40566

Closed

matriv added a commit that referenced this pull request Apr 1, 2019

SQL: Introduce SQL TIME data type (#39802)

899ed2b

Support ANSI SQL's TIME type by introductin a runtime-only ES SQL time type. Closes: #38174 (cherry picked from commit 046ccd4)

gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019

SQL: Introduce SQL TIME data type (elastic#39802)

22bb77d

Support ANSI SQL's TIME type by introductin a runtime-only ES SQL time type. Closes: elastic#38174

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		protected abstract Object doFold(ZonedDateTime dateTime);


		protected Object doFold(OffsetTime time) {



		timeAsHavingFilter
		SELECT MINUTE_OF_HOUR(MAX(birth_date)::TIME + INTERVAL 10 MINUTES) as minute, gender FROM test_emp GROUP BY gender HAVING CAST(MAX(birth_date) AS TIME) = CAST('00:00:00.000' AS TIME) ORDER BY gender;

SQL: Introduce SQL TIME data type #39802

SQL: Introduce SQL TIME data type #39802

Uh oh!

Conversation

matriv commented Mar 7, 2019

Uh oh!

elasticmachine commented Mar 7, 2019

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!