-
Notifications
You must be signed in to change notification settings - Fork 25.6k
SQL: Implement CURRENT_TIME/CURTIME functions #40662
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
Conversation
|
Pinging @elastic/es-search |
|
Needs to wait for #39802 and be rebased on top. |
costin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is -> which are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a NOTE: or WARNING:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use them in the limitations page. In the grouping.asciidoc there is and IMPORTANT paragraph mentioning the same limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative is to move these tests back to the docs.csv-spec and mark them as -Ignore which means they will not be ran.
After `TIME` SQL data type is introduced, implement `CURRENT_TIME/CURTIME` functions similar to CURRENT_TIMESTAMP that return the system's current time (only, without the date part). Closes: elastic#40468
astefan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left few comments regarding the documentation.
| -------------------------------------------------- | ||
|
|
||
| ["source","sql",subs="attributes,callouts,macros"] | ||
| -------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Twice the same query? (see above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be an example for CURTIME, fixing it.
| [source, sql] | ||
| -------------------------------------------------- | ||
| CURRENT_TIME | ||
| CURRENT_TIME(precision <1>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were using the CURRENT_TIME([precision <1>]) (square brackets) for optional parameters, but looking at CURRENT_TIMESTAMP I see the same approach. Other functions (ROUND, TRUNCATE and few string functions) use the square brackets format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the square brackets, will also fix it for CURRENT_TIMESTAMP with a separate PR.
| -------------------------------------------------- | ||
|
|
||
| [IMPORTANT] | ||
| Currently, Using a _precision_ greater than 3 doesn't make any difference to the output of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, lowercase Using.
After `TIME` SQL data type is introduced, implement `CURRENT_TIME/CURTIME` functions similarly to CURRENT_TIMESTAMP that return the system's current time (only, without the date part). Closes: elastic#40468
After
TIMESQL data type is introduced, implementCURRENT_TIME/CURTIMEfunctions similar to CURRENT_TIMESTAMPthat return the system's current time (only, without the date part).
Closes: #40648