-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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 DATEADD function #47747
Conversation
Implement DATEADD/TIMESTAMPADD function as per the MS-SQL spec: https://docs.microsoft.com/en-us/sql/t-sql/functions/dateadd-transact-sql?view=sql-server-2017 which allows a user to add/subtract specified number of specified units to/from a date/datetime field/expression. Split BinaryDateTimePipe into 2 DatePart/DateTrunc Pipe classes, so that the BinaryDateTimeOperation enum can be removed. Closes: elastic#47746
Pinging @elastic/es-search (:Search/SQL) |
Opened a separate PR for the binary functions refactoring: #47786 which should be merged first. |
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
@@ -248,6 +248,79 @@ include-tagged::{sql-specs}/docs/docs.csv-spec[filterNow] | |||
Currently, using a _precision_ greater than 3 doesn't make any difference to the output of the | |||
function as the maximum number of second fractional digits returned is 3 (milliseconds). | |||
|
|||
[[sql-functions-datetime-add]] | |||
==== `DATE_ADD/DATE_ADD/TIMESTAMPADD/TIMESTAMP_ADD` |
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.
DATE_ADD
appears twice, one probably should be DATEADD
.
.Synopsis: | ||
[source, sql] | ||
-------------------------------------------------- | ||
DATE_PART( |
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.
DATE_PART
?
*Input*: | ||
|
||
<1> string expression denoting the unit to add to the date/datetime | ||
<2> integer expression denoting how many times the above unit should be added/subtracted (if it's negative) |
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.
Instead of saying added/subtracted (if it's negative)
which can be confusing, explain the situation a bit.
added (negative values are allowed resulting in a subtraction
. Or make another sentence - subtraction is supported through the use of negative values
.
|
||
.Description: | ||
|
||
Add/Subtract the given number of units to/from a date/datetime. If any of the three arguments is `null` |
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.
Again, it's DATEADD
- I get it you can effectively subtract by using negative values by using Add/Subtract
implies one can do subtraction of a negative value as well which is not the case.
In other words there's only addition of both positive and negative values but never a subtraction.
@@ -235,7 +236,7 @@ public static Double asin(Number value) { | |||
public static Double atan(Number value) { | |||
return MathOperation.ATAN.apply(value); | |||
} | |||
|
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.
?
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.
Thought is ok to fix the formatting since it's only touching empty lines.
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.
Looking great. I've left some minor comments.
Also, I would like to see some more complex integration tests. For example:
SELECT first_name, gender, hire_date, DATE_ADD(CASE WHEN gender = 'M' THEN CONCAT(gender, 'onths') WHEN gender = 'F' THEN 'year' ELSE 'quarter' END, 5, hire_date + INTERVAL 10 month) AS dp, YEAR(DATE_ADD(CASE WHEN gender = 'M' THEN CONCAT(gender, 'onths') WHEN gender = 'F' THEN 'year' ELSE 'quarter' END, 5, hire_date + INTERVAL 10 month)) FROM test_emp WHERE YEAR(dp) > 1990 AND first_name LIKE '%y%' ORDER BY dp DESC LIMIT 15
.SELECT languages, first_name, gender, hire_date, CAST(DATE_ADD(CASE WHEN gender = 'M' THEN CONCAT(gender, 'onths') WHEN gender = 'F' THEN NULL ELSE 'quarter' END, 5, hire_date + INTERVAL 10 month) AS DATE) AS dp FROM test_emp WHERE languages >= 3 AND first_name LIKE '%y%' ORDER BY dp ASC, languages DESC
@@ -248,6 +248,79 @@ include-tagged::{sql-specs}/docs/docs.csv-spec[filterNow] | |||
Currently, using a _precision_ greater than 3 doesn't make any difference to the output of the | |||
function as the maximum number of second fractional digits returned is 3 (milliseconds). | |||
|
|||
[[sql-functions-datetime-add]] | |||
==== `DATE_ADD/DATE_ADD/TIMESTAMPADD/TIMESTAMP_ADD` |
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.
DATE_ADD/DATE_ADD
.Synopsis: | ||
[source, sql] | ||
-------------------------------------------------- | ||
DATE_PART( |
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.
DATE_PART
?
*Input*: | ||
|
||
<1> string expression denoting the unit to add to the date/datetime | ||
<2> integer expression denoting how many times the above unit should be added/subtracted (if it's negative) |
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.
added (if it's positive) or subtracted (it it's negative)
|
||
*Input*: | ||
|
||
<1> string expression denoting the unit to add to the date/datetime |
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 would say "the time unit".
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'm changing to date/time unit
.
null | 2004-04-30 00:00:00.000Z | ||
F | 2002-05-19 00:00:00.000Z | ||
; | ||
|
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.
All the integration tests seem to use DATE_ADD
variant only. Can you mix and match the functions' aliases for completeness sake, as well?
|
||
public static final String NAME = "dtadd"; | ||
|
||
public DateAddProcessor(Processor source1, Processor source2, Processor source3, ZoneId zoneId) { |
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.
Can you name these parameters like "datePart", "timeUnit", "dateField" or similar? (like you did in the rest of the class)
@Override | ||
public void testTransform() { | ||
// test transforming only the properties (source, expression), | ||
// skipping the children (the two parameters of the binary function) which are tested separately |
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.
Does this comment apply to this Pipe?
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.
Copy-paste the source of all evil :(
Implement DATEADD/TIMESTAMPADD function as per the MS-SQL spec: https://docs.microsoft.com/en-us/sql/t-sql/functions/dateadd-transact-sql?view=sql-server-2017 which allows a user to add/subtract specified number of specified units to/from a date/datetime field/expression. Closes: #47746 (cherry picked from commit e624bc2)
Implement DATEADD/TIMESTAMPADD function as per the MS-SQL spec:
https://docs.microsoft.com/en-us/sql/t-sql/functions/dateadd-transact-sql?view=sql-server-2017
which allows a user to add/subtract specified number of specified units
to/from a date/datetime field/expression.
Split BinaryDateTimePipe into 2 DatePart/DateTrunc Pipe classes, so that
the BinaryDateTimeOperation enum can be removed.
Closes: #47746