SQL: Implement CURRENT_DATE #38175

matriv · 2019-02-01T11:06:30Z

Since DATE data type is now available, this implements the
CURRENT_DATE/CURRENT_DATE()/TODAY() similar to CURRENT_TIMESTAMP.

Closes: #38160

elasticmachine · 2019-02-01T11:06:31Z

Pinging @elastic/es-search

astefan · 2019-02-01T14:33:03Z

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

CURRENT_TIMESTAMP - typo?

astefan · 2019-02-01T14:34:17Z

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

How about This method always returns the same value within a query, no matter how many times it is used inside the query. to make it more clear?

astefan · 2019-02-01T14:35:41Z

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

Here, the same comment about the clarity of "same value within a query"... as above.

astefan · 2019-02-01T14:44:04Z

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

Here I would test this differently so that the result of the query is not the test above it, meaning subtracting 20+ years (to get into 1900s) and using a TRUNCATE without parameter to get a result of 1.

astefan

LGTM, left some comments though.

astefan · 2019-02-01T14:45:32Z

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

Put YEAR(hire_date) - YEAR(TODAY()) / 1000 as a returned value as well to prove the WHERE condition was applied correctly.

astefan · 2019-02-01T14:47:52Z

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

Will this look too crowded if you also add hire_date and TODAY() - INTERVAL 25 YEARS as returned values? I'm ok with it as is, but if it doesn't look too much I would make that change as well.

Imho I wouldn't do that here, because it's docs and yep seem a bit "too crowded".

astefan · 2019-02-01T14:58:34Z

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

Why not integrating the datePrecision method directly in the constructor, as it seems it's only used here and it's not complex: this.date = DateUtils.asDateOnly(configuration().now()) as I don't think now() can be null at this stage. Some other pieces of code would break before this one while getting the "now" value.

The null check is there for the NodeSubClassTests.

costin

Looks good (forgot about TODAY). Left a couple of comments though regarding the function and the grammar which could be improved.

costin · 2019-02-01T17:01:46Z

x-pack/plugin/sql/src/main/antlr/SqlBase.g4

Having a separate section for each method is just noise.
We should either define the method directly as we do with CAST or define all time related methods under builtinDateTimeFunction.

I thought about it, as I saw your intention there with the builtinDateTimeFunction. But couldn't think of a way to do it in the parser as we don't have CURRENT DATE and CURRENT TIMESTAMP but CURRENT_DATE and CURRENT_TIMESTAMP. if we try to have a common prefix then the lexer will allow whitespace so things like:

CURRENT_ DATE

We don't need a common prefix, just to group merge the different "groups" into one, e.g:

: name=CURRENT_TIMESTAMP ('(' precision=INTEGER_VALUE? ')')? #currentTimestamp | name=CURRENT_DATE ('(' ')')? #currentDate

costin · 2019-02-01T17:06:35Z

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

Instead of providing a new function, the logic from CurrentDateTime should be reused - I would refactor that a package protected CurrentFunction and have CurrentDate/Timestamp extensions that would just trim the precision in the delegating constructor as well as specifying the dedicated type.

I don't see a nice way to do it unless the date attribute in the new abstract class is not final and is not set in the constructor.

public CurrentDate(Source, Configuration) { super(source, trimAsDate(configuration.now()), DataType.DATE); }

?

The base class would only accept the source and the ZonedDateTime while each subclass would take care of extracting the proper date from the configuration and specify the type.

You cannot do a method call in the super(), it's not allowed.

You can if the method is static - a constructor is simply a special method. One is not allowed to call a (virtual) method from the about to be created class:
super(source, DateUtils.asDateOnly(configuration.now()), DataType.DATE);

costin · 2019-02-02T12:23:24Z

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

public CurrentDate(Source, Configuration) { super(source, trimAsDate(configuration.now()), DataType.DATE); }

?

The base class would only accept the source and the ZonedDateTime while each subclass would take care of extracting the proper date from the configuration and specify the type.

costin · 2019-02-02T12:26:18Z

x-pack/plugin/sql/src/main/antlr/SqlBase.g4

We don't need a common prefix, just to group merge the different "groups" into one, e.g:

: name=CURRENT_TIMESTAMP ('(' precision=INTEGER_VALUE? ')')? #currentTimestamp | name=CURRENT_DATE ('(' ')')? #currentDate

Since DATE data type is now available, this implements the `CURRENT_DATE/CURRENT_DATE()/TODAY()` similar to `CURRENT_TIMESTAMP`. Closes: elastic#38160

matriv · 2019-02-02T14:30:08Z

@costin @astefan Addressed comments.
Also added tests for ORDER BY and GROUP BY NOW() and TODAY().

matriv · 2019-02-02T16:41:44Z

@elasticmachine run elasticsearch-ci/2

costin · 2019-02-02T21:37:07Z

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

You can if the method is static - a constructor is simply a special method. One is not allowed to call a (virtual) method from the about to be created class:
super(source, DateUtils.asDateOnly(configuration.now()), DataType.DATE);

costin · 2019-02-02T21:38:11Z

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

+    public CurrentDate(Source source, Configuration configuration) {
+        super(source, configuration, DataType.DATE);
+        ZonedDateTime zdt = configuration().now();
+        if (zdt == null) {


A null zdt is likely a bug in the configuration - it means there's no now() which is ... not possible.

costin · 2019-02-02T21:39:15Z

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

-        
+
        switch (ctx.name.getType()) {
+            case SqlBaseLexer.CURRENT_DATE:


matriv · 2019-02-03T13:59:01Z

@costin fixup pushed

costin

LGTM

costin · 2019-02-05T07:37:07Z

...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/CurrentDateTime.java

-                return zdt.withNano(nano);
-            }
+    static ZonedDateTime nanoPrecision(ZonedDateTime zdt, Expression precisionExpression) {
+        int precision = precisionExpression != null ? ((Number) precisionExpression.fold()).intValue() : 0;


Use Foldables.intValueOf instead as it triggers a full conversion and also checks the foldability of the expression.

matriv · 2019-02-05T10:38:23Z

@elasticmachine run elasticsearch-ci/default-distro

matriv · 2019-02-05T10:39:41Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2

matriv · 2019-02-05T11:19:27Z

@elasticmachine run elasticsearch-ci/default-distro

matriv · 2019-02-05T12:33:52Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2

matriv · 2019-02-05T13:22:43Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

matriv · 2019-02-05T14:22:19Z

@elasticmachine run elasticsearch-ci/2

Since DATE data type is now available, this implements the `CURRENT_DATE/CURRENT_DATE()/TODAY()` similar to `CURRENT_TIMESTAMP`. Closes: #38160

matriv · 2019-02-05T16:29:21Z

Backported to 6.x with 62933db

* master: (23 commits) Lift retention lease expiration to index shard (elastic#38380) Make Ccr recovery file chunk size configurable (elastic#38370) Prevent CCR recovery from missing documents (elastic#38237) re-enables awaitsfixed datemath tests (elastic#38376) Types removal fix FullClusterRestartIT warnings (elastic#38445) Make sure to reject mappings with type _doc when include_type_name is false. (elastic#38270) Updates the grok patterns to be consistent with logstash (elastic#27181) Ignore type-removal warnings in XPackRestTestHelper (elastic#38431) testHlrcFromXContent() should respect assertToXContentEquivalence() (elastic#38232) add basic REST test for geohash_grid (elastic#37996) Remove DiscoveryPlugin#getDiscoveryTypes (elastic#38414) Fix the clock resolution to millis in GetWatchResponseTests (elastic#38405) Throw AssertionError when no master (elastic#38432) `if_seq_no` and `if_primary_term` parameters aren't wired correctly in REST Client's CRUD API (elastic#38411) Enable CronEvalToolTest.testEnsureDateIsShownInRootLocale (elastic#38394) Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor. (elastic#38129) SQL: Implement CURRENT_DATE (elastic#38175) Mute testReadRequestsReturnLatestMappingVersion (elastic#38438) [ML] Report index unavailable instead of waiting for lazy node (elastic#38423) Update Rollup Caps to allow unknown fields (elastic#38339) ...

* 6.x: (25 commits) Backport of types removal for Put/Get index templates (elastic#38465) Add support for API keys to access Elasticsearch (elastic#38291) (elastic#38399) Deprecate support for internal versioning for concurrency control (elastic#38451) Deprecate types in rollover index API (elastic#38389) (elastic#38458) Add typless client side GetIndexRequest calls and response class (elastic#38422) [ML] Report index unavailable instead of waiting for lazy node (elastic#38444) await fix CurtIT#testIndex until elastic#38451 is merged (elastic#38466) Update ilm-api.asciidoc, point to REMOVE policy (elastic#38235) (elastic#38464) SQL: Fix esType for DATETIME/DATE and INTERVALS (elastic#38179) Clean up duplicate follow config parameter code (elastic#37688) (elastic#38443) Deprecation check for No Master Block setting (elastic#38383) Bubble-up exceptions from scheduler (elastic#38441) Lift retention lease expiration to index shard (elastic#38391) Deprecate maxRetryTimeout in RestClient and increase default value (elastic#38425) Update Rollup Caps to allow unknown fields (elastic#38446) Backport of elastic#38411: `if_seq_no` and `if_primary_term` parameters aren't wired correctly in REST Client's CRUD API Support unknown fields in ingest pipeline map configuration (elastic#38429) SQL: Implement CURRENT_DATE (elastic#38175) Backport changes to the release notes script. (elastic#38346) Fix ILM explain response to allow unknown fields (elastic#38363) ...

matriv added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.7.0 labels Feb 1, 2019

matriv requested review from astefan and costin February 1, 2019 11:06

matriv force-pushed the mt/38160 branch from 78e6fe5 to e81a238 Compare February 1, 2019 11:07

astefan reviewed Feb 1, 2019

View reviewed changes

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

Copy link

Contributor

astefan Feb 1, 2019

Choose a reason for hiding this comment

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

CURRENT_TIMESTAMP - typo?

matriv reacted with thumbs up emoji

astefan reviewed Feb 1, 2019

View reviewed changes

astefan approved these changes Feb 1, 2019

View reviewed changes

costin requested changes Feb 1, 2019

View reviewed changes

costin requested changes Feb 2, 2019

View reviewed changes

matriv added 2 commits February 2, 2019 16:28

SQL: Implement CURRENT_DATE

abf331e

Since DATE data type is now available, this implements the `CURRENT_DATE/CURRENT_DATE()/TODAY()` similar to `CURRENT_TIMESTAMP`. Closes: elastic#38160

Address comments

26cca18

matriv force-pushed the mt/38160 branch from 5bdcf6f to 26cca18 Compare February 2, 2019 14:28

costin requested changes Feb 2, 2019

View reviewed changes

Address comments

66a5335

costin approved these changes Feb 5, 2019

View reviewed changes

address comment

f5e2347

Merge remote-tracking branch 'upstream/master' into mt/38160

fd86558

Fix code after merge

480ccd6

matriv merged commit c9701be into elastic:master Feb 5, 2019

matriv deleted the mt/38160 branch February 5, 2019 16:15

matriv added a commit that referenced this pull request Feb 5, 2019

SQL: Implement CURRENT_DATE (#38175)

62933db

Since DATE data type is now available, this implements the `CURRENT_DATE/CURRENT_DATE()/TODAY()` similar to `CURRENT_TIMESTAMP`. Closes: #38160

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



		switch (ctx.name.getType()) {
		case SqlBaseLexer.CURRENT_DATE:

SQL: Implement CURRENT_DATE #38175

SQL: Implement CURRENT_DATE #38175

Uh oh!

Conversation

matriv commented Feb 1, 2019

Uh oh!

elasticmachine commented Feb 1, 2019

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!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

matriv commented Feb 2, 2019

Uh oh!

matriv commented Feb 2, 2019

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!

matriv commented Feb 3, 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!

matriv commented Feb 5, 2019

Uh oh!

matriv commented Feb 5, 2019

Uh oh!

matriv commented Feb 5, 2019

Uh oh!

matriv commented Feb 5, 2019

Uh oh!

matriv commented Feb 5, 2019

matriv Feb 1, 2019 •

edited

Loading