-
Notifications
You must be signed in to change notification settings - Fork 141
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
Update DATE_ADD
/ADDDATE
and DATE_SUB
/SUBDATE
functions. (#122)
#1182
Update DATE_ADD
/ADDDATE
and DATE_SUB
/SUBDATE
functions. (#122)
#1182
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1182 +/- ##
============================================
- Coverage 98.35% 95.91% -2.45%
- Complexity 3604 3607 +3
============================================
Files 344 354 +10
Lines 8933 9592 +659
Branches 562 681 +119
============================================
+ Hits 8786 9200 +414
- Misses 142 334 +192
- Partials 5 58 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
core/src/main/java/org/opensearch/sql/data/model/ExprTimestampValue.java
Outdated
Show resolved
Hide resolved
* (DATE, LONG) -> DATE | ||
* (TIME/DATETIME/TIMESTAMP, LONG) -> DATETIME | ||
*/ | ||
private Stream<SerializableFunction<?, ?>> get_adddate_subdate_signatures( |
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.
seems cratedb normalized to arthemetic operation, https://github.com/crate/crate/blob/5.0/server/src/main/java/io/crate/expression/scalar/arithmetic/ArithmeticFunctions.java
could we do something similar?
impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), | ||
private Stream<SerializableFunction<?, ?>> get_date_add_date_sub_signatures( | ||
SerializableTriFunction<FunctionProperties, ExprValue, ExprValue, ExprValue> function) { | ||
return Stream.of( |
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 Datetime required? I found you post same concern in #1176.
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.
Unfortunately, yes, unless we have 4 types for date/time.
679bf19
to
99c3708
Compare
Rebased to resolve conflicts |
Resolved merge conflicts. |
... and again. |
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
…ulations Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand yuryf@bitquilltech.com
Description
I referred to MySQL docs and tried to reproduce MySQL v.8.0.30 behavior as a reference.
See team review and discussion in Bit-Quill#122
Rework on
DATE_ADD
/ADDDATE
functions.Changes
Fixes:
TIME
, considering today's date. If argument isDATE
, it interpreted as midnight of thisDATE
.DATETIME
.Future changes (TODOs):
STRING
Removed function
DATE_ADD(date, int)
Left function (matches MySQL):
Update signature list
TIME
STRING
- all datetime types have implicit cast from a string alreadyMore details
DATE_ADD
ADDDATE
Rework on
DATE_SUB
/SUBDATE
functions.Changes
Removed function
DATE_SUB(date, int)
The same fixes and notes as for
DATE_ADD
/ADDDATE
functions.Test queries:
NB: Due to #853 you can't test with
DATETIME
(without code changes), but I tested with itOpenSearch
MySQL
PostgreSQL
Test data
I found that first 6 rows from
date0
,time0
,time1
,datetime0
are good for testing - these columns have different data types in MySQL. In OpenSearch SQL all[date][time]
columns havetimestamp
type, so I useCAST
for clear testing.data
Related issue:
Fixes #291
https://forum.opensearch.org/t/subdate-date-sub-query-method-not-supported/9252/2
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.