SQL: Rename SQL type DATE to DATETIME #37395

matriv · 2019-01-13T21:25:43Z

SQL data type DATE has only the date part (e.g.: 2019-01-14)
without any time information. Previously the SQL type DATE was
referring to the ES DATE which contains also the time part along
with TZ information. To conform with SQL data types the data type
DATE is renamed to DATETIME, since it includes also the time,
as a new runtime SQL DATE data type will be introduced down the road,
which only contains the date part and meets the SQL standard.

Closes: #36440

elasticmachine · 2019-01-13T21:25:45Z

Pinging @elastic/es-search

SQL data type DATE has only the date part (e.g.: 2019-01-14) without any time information. Previously the SQL type DATE was referring to the ES DATE which contains also the time part along with TZ information. To conform with SQL data types the data type `DATE` is renamed to `DATETIME`, since it includes also the time, as a new runtime SQL `DATE` data type will be introduced down the road, which only contains the date part and meets the SQL standard. Closes: elastic#36440

costin

I've requested some small changes however they should be fairly obvious; once their fixed I'm fine with merging the PR.

costin · 2019-01-14T18:18:53Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java

        } catch (IllegalArgumentException ex) {
+            if (uppercase.equals("DATE")) {


Please move the if before the try - that is first check if it's a date, otherwise just do the typical DataType conversion.

costin · 2019-01-14T21:12:12Z

docs/reference/sql/language/data-types.asciidoc

+| <<keyword, `keyword`>>      | varchar               | based on <<ignore-above>>
+| <<text, `text`>>            | varchar               | 2,147,483,647
+| <<binary, `binary`>>        | varbinary             | 2,147,483,647
+| <<date, `date`>>            | timestamp/datetime    | 24


Maybe it makes sense to add another column so the table contains ES/ES-SQL/SQL types. It might be redundant but it might make the datetime case clearer - can be done in a separate PR.

Opened issue to track it: #37519

costin · 2019-01-14T21:13:41Z

.../src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

            error("SELECT AVG(date) FROM test"));
    }

    public void testNotSupportedAggregateOnString() {
-        assertEquals("1:8: [MAX(keyword)] argument must be [numeric or date], found value [keyword] type [keyword]",
+        assertEquals("1:8: [MAX(keyword)] argument must be [NUMERIC or DATETIME], found value [keyword] type [KEYWORD]",


What's with the uppercasing of the types? It is used as lowercase since that's the convention used by ES itself.

Hm, My bad. I was trying to make it consistent with those:
https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java#L336
but I guess we should change those to lowercase instead. Will revert.

costin · 2019-01-14T21:15:01Z

docs/reference/sql/functions/date-time.asciidoc

@@ -22,9 +22,9 @@ The table below shows the mapping between {es} and {es-sql}:
 |==========================
 s|{es}
 s|{es-sql}
-2+h| Index/Table date math
+2+h| Index/Table date/time math


I would use datetime in one word - / suggest one can pick either date or time and the latter it's not actually available.

costin · 2019-01-14T21:19:00Z

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

@@ -190,6 +190,6 @@ public static TypeResolution typeMustBe(Expression e,
                    paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : " " + paramOrd.name().toLowerCase(Locale.ROOT),
                    Strings.arrayToDelimitedString(acceptedTypes, " or "),
                    Expressions.name(e),
-                    e.dataType().esType));
+                    e.dataType().esType.toUpperCase(Locale.ROOT)));


I think we should keep the lower casing in place since that's how the types are defined in ES as well (keyword vs KEYWORD).
If anything, we should change for example the column descriptors to always return the ES types in lowercase to make the context (SQL type vs ES type) even clearer.

costin · 2019-01-14T21:21:38Z

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

@@ -28,13 +28,13 @@ DESCRIBE test_alias;

       column       |     type      |    mapping    
 --------------------+---------------+---------------
-birth_date          |TIMESTAMP      |DATE           
+birth_date          |TIMESTAMP      |DATETIME


Ideally we should change mapping to be lower-case to follow the ES convention - this can be done in a follow-up PR.

birth_date | TIMESTAMP | datetime dep | STRUCT | nested

Tracked here: #37521

astefan

LGTM

matriv · 2019-01-16T12:12:10Z

run the gradle build tests 1

matriv · 2019-01-16T12:13:13Z

run the gradle build tests 2

matriv · 2019-01-16T13:03:25Z

run the gradle build tests 2

matriv · 2019-01-16T13:03:31Z

run the gradle build tests 1

costin · 2019-01-16T14:11:20Z

run the gradle build tests 2

matriv · 2019-01-16T16:05:23Z

run the gradle build tests 2

matriv · 2019-01-16T16:05:29Z

run the gradle build tests 1

matriv · 2019-01-16T17:24:02Z

run the gradle build tests 1

matriv · 2019-01-16T17:43:10Z

run the gradle build tests 1

matriv · 2019-01-16T18:05:08Z

run the gradle build tests 2

matriv · 2019-01-16T20:11:40Z

run the gradle build tests 2

matriv · 2019-01-16T20:11:45Z

run the gradle build tests 1

matriv · 2019-01-16T20:12:46Z

run the gradle build tests 1

matriv · 2019-01-16T20:15:13Z

run the gradle build tests 1

matriv · 2019-01-16T20:35:23Z

run the gradle build tests 2

costin · 2019-01-16T21:01:07Z

run the gradle build tests 1

matriv · 2019-01-16T21:18:42Z

run the gradle build tests 2

matriv · 2019-01-16T21:40:51Z

run the gradle build tests 2

* SQL: Rename SQL data type DATE to DATETIME SQL data type DATE has only the date part (e.g.: 2019-01-14) without any time information. Previously the SQL type DATE was referring to the ES DATE which contains also the time part along with TZ information. To conform with SQL data types the data type `DATE` is renamed to `DATETIME`, since it includes also the time, as a new runtime SQL `DATE` data type will be introduced down the road, which only contains the date part and meets the SQL standard. Closes: #36440 * Address comments

matriv · 2019-01-17T08:47:35Z

Backported to 6.x with 2a13652

…-response-header-performance * elastic/master: Remove Redundant RestoreRequest Class (elastic#37535) Create specific exception for when snapshots are in progress (elastic#37550) Mute UnicastZenPingTests#testSimplePings [DOCS] Adds size limitation to the get datafeeds APIs (elastic#37578) Fix assertion at end of forceRefreshes (elastic#37559) Propagate Errors in executors to uncaught exception handler (elastic#36137) [DOCS] Adds limitation to the get jobs API (elastic#37549) Add set_priority action to ILM (elastic#37397) Make recovery source send operations non-blocking (elastic#37503) Allow field types to optimize phrase prefix queries (elastic#37436) Added fatal_exception field for ccr stats in monitoring mapping. (elastic#37563) Fix testRelocateWhileContinuouslyIndexingAndWaitingForRefresh (elastic#37560) Moved ccr integration to the package with other ccr integration tests. Mute TransportClientNodesServiceTests#testListenerFailures Decreased time out in test Fix erroneous docstrings for abstract bulk by scroll request (elastic#37517) SQL: Rename SQL type DATE to DATETIME (elastic#37395) Remove the AbstracLifecycleComponent constructor with Settings (elastic#37523)

matriv added >non-issue WIP v7.0.0 :Analytics/SQL SQL querying v6.7.0 labels Jan 13, 2019

matriv changed the title ~~WIP~~ SQL: Rename SQL type DATE to DATETIME Jan 14, 2019

matriv force-pushed the mt/fix-36440 branch from 5823a7d to b38d78a Compare January 14, 2019 18:34

matriv requested review from costin and astefan January 14, 2019 18:36

matriv removed the WIP label Jan 14, 2019

costin approved these changes Jan 14, 2019

View reviewed changes

astefan approved these changes Jan 14, 2019

View reviewed changes

matriv mentioned this pull request Jan 16, 2019

SQL: Add protocol tests and remove jdbc_type from drivers response #37516

Merged

matriv and others added 2 commits January 16, 2019 12:37

Address comments

57c3609

Merge remote-tracking branch 'upstream/master' into mt/fix-36440

4d59348

Merge remote-tracking branch 'upstream/master' into mt/fix-36440

e34437b

Merge remote-tracking branch 'upstream/master' into mt/fix-36440

7a1d635

Merge remote-tracking branch 'upstream/master' into mt/fix-36440

51b67f6

matriv merged commit 1686c32 into elastic:master Jan 17, 2019

matriv deleted the mt/fix-36440 branch January 17, 2019 08:18

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Rename SQL type DATE to DATETIME #37395

SQL: Rename SQL type DATE to DATETIME #37395

matriv commented Jan 13, 2019 •

edited

Loading

elasticmachine commented Jan 13, 2019

costin left a comment

costin Jan 14, 2019

costin Jan 14, 2019

matriv Jan 16, 2019

costin Jan 14, 2019

matriv Jan 14, 2019

costin Jan 14, 2019

costin Jan 14, 2019

costin Jan 14, 2019

matriv Jan 16, 2019

astefan left a comment

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

costin commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

costin commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 17, 2019

		} catch (IllegalArgumentException ex) {
		if (uppercase.equals("DATE")) {

SQL: Rename SQL type DATE to DATETIME #37395

SQL: Rename SQL type DATE to DATETIME #37395

Conversation

matriv commented Jan 13, 2019 • edited Loading

elasticmachine commented Jan 13, 2019

costin 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

astefan left a comment

Choose a reason for hiding this comment

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

costin commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

costin commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 16, 2019

matriv commented Jan 17, 2019

matriv commented Jan 13, 2019 •

edited

Loading