Skip to content
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.

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

SQL: Implement CURRENT_DATE #38175

Merged
merged 6 commits into from
Feb 5, 2019
Merged

SQL: Implement CURRENT_DATE #38175

merged 6 commits into from
Feb 5, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Feb 1, 2019

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

Closes: #38160

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@@ -93,6 +93,48 @@ include-tagged::{sql-specs}/docs.csv-spec[dtIntervalMul]

beta[]

[[sql-functions-current-date]]
==== `CURRENT_TIMESTAMP/CURDATE`
Copy link
Contributor

Choose a reason for hiding this comment

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

CURRENT_TIMESTAMP - typo?

[NOTE]
Unlike CURRENT_DATE, `CURDATE()` can only be used as a function with no arguments and not as a keyword.

This method always returns the same value within a query.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

.Description:

This function offers the same functionality as <<sql-functions-current-date,CURRENT_DATE()>> function: returns
the date when the current query reached the server. This method always returns the same value within a query.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

;

todayIntervalSubstraction
SELECT YEAR(TODAY() - INTERVAL 2 YEARS) / 1000 AS result;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 some comments though.

;

currentDateFilterScript
SELECT first_name FROM test_emp WHERE YEAR(hire_date) - YEAR(TODAY()) / 1000 > 10 ORDER BY first_name ASC LIMIT 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


filterToday
// tag::filterToday
SELECT first_name FROM emp WHERE hire_date > TODAY() - INTERVAL 25 YEARS ORDER BY first_name ASC LIMIT 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


public CurrentDate(Source source, Configuration configuration) {
super(source, configuration, DataType.DATE);
this.date = datePrecision(configuration().now());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The null check is there for the NodeSubClassTests.

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.

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

@@ -215,6 +215,7 @@ valueExpression
primaryExpression
: castExpression #cast
| extractExpression #extract
| builtinDateFunction #currentDateFunction
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@matriv matriv Feb 1, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


private final ZonedDateTime date;

public CurrentDate(Source source, Configuration configuration) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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);


private final ZonedDateTime date;

public CurrentDate(Source source, Configuration configuration) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -215,6 +215,7 @@ valueExpression
primaryExpression
: castExpression #cast
| extractExpression #extract
| builtinDateFunction #currentDateFunction
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

matriv commented Feb 2, 2019

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

@matriv
Copy link
Contributor Author

matriv commented Feb 2, 2019

@elasticmachine run elasticsearch-ci/2


private final ZonedDateTime date;

public CurrentDate(Source source, Configuration configuration) {
Copy link
Member

Choose a reason for hiding this comment

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

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);

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

👍

@matriv
Copy link
Contributor Author

matriv commented Feb 3, 2019

@costin fixup pushed

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

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

Choose a reason for hiding this comment

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

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

@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

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

@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

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

@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

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

@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

@elasticmachine run elasticsearch-ci/2

@matriv matriv merged commit c9701be into elastic:master Feb 5, 2019
@matriv matriv deleted the mt/38160 branch February 5, 2019 16:15
matriv added a commit that referenced this pull request Feb 5, 2019
Since DATE data type is now available, this implements the
`CURRENT_DATE/CURRENT_DATE()/TODAY()` similar to `CURRENT_TIMESTAMP`.

Closes: #38160
@matriv
Copy link
Contributor Author

matriv commented Feb 5, 2019

Backported to 6.x with 62933db

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 5, 2019
* 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)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 6, 2019
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants