-
Notifications
You must be signed in to change notification settings - Fork 0
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
Extend comparison methods to accept different datetime types. #129
Extend comparison methods to accept different datetime types. #129
Conversation
Codecov Report
@@ Coverage Diff @@
## integ-datetime-comparison #129 +/- ##
===============================================================
+ Coverage 95.78% 95.80% +0.01%
- Complexity 3503 3538 +35
===============================================================
Files 350 350
Lines 9310 9345 +35
Branches 669 676 +7
===============================================================
+ Hits 8918 8953 +35
Misses 334 334
Partials 58 58
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
.../src/main/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperator.java
Outdated
Show resolved
Hide resolved
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.
BinaryPredicateOperator.less
, .greater
, .equals
, etc differ only in a few places.
You could write one operatorImpl
method parametrized by the differences and use it.
Fixed in d4425e6. |
core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java
Outdated
Show resolved
Hide resolved
@@ -73,7 +73,7 @@ public boolean equal(ExprValue o) { | |||
*/ | |||
@Override | |||
public int compare(ExprValue other) { | |||
return Integer.compare(valueList.size(), other.collectionValue().size()); | |||
return equal(other) ? 0 : 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.
Is this identical implementation in ExprTupleValue
needed twice? Why not pull implementation to AbstractExprValue
?
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 reverted this in d6f68b9.
@@ -51,6 +56,21 @@ public LocalTime timeValue() { | |||
return time; | |||
} | |||
|
|||
@Override | |||
public LocalDate dateValue() { | |||
return LocalDate.now(); |
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 use the curdate date value. If dateValue()
is called at the wrong time, it can lead to unexpected behaviour.
For example, consider SELECT CAST(TIME('22:00:00') AS DATETIME) > CAST(TIME('21:00:00'))
.
Let's say today's date is December 1, 2022. Then the query will be the same as SELECT DATETIME('2022-12-01 22:00:00') > DATETIME('2022-12-01 21:00:00')
and would evaluate to true
.
However, if it's executed close enough to midnight, it's possible to get December 1st as current date for the first cast, and December 2nd for the second. Then the query will be same as SELECT DATETIME('2022-12-01 22:00:00') > DATETIME('2022-12-02 21:00:00')
and suddenly evaluate to false
.
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.
As we discussed that will be a part of another feature.
+ "`neq3` = DATE('1961-04-12') != DATE('1961-04-12'), " | ||
+ "`gt1` = DATE('1984-12-15') > DATE('1961-04-12'), " | ||
+ "`gt2` = DATE('1984-12-15') > DATE('2020-09-16'), " | ||
+ "`lt1` = DATE('1961-04-12') < DATE('1984-12-15'), " |
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.
It would help reviews a lot if each test case was a separate query.
As it is written, to check the expected results for, say lt1, I have to count which field it's in the response -- 8, and then count to the 8th parameter of rows
on line 72. If it was one query -- source =%s | eval eq1 = DATE('1961-04-12') < DATE('1984-12-15') | fields eq1` -- it's obvious.
Test like these are prime candidates for parametarization.
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.
Fixed in b6371b6.
v1.type() == TIME ? v1.timeValue().atDate(LocalDate.now()) : v1.datetimeValue(), | ||
v2.type() == TIME ? v2.timeValue().atDate(LocalDate.now()) : v2.datetimeValue()); | ||
} else if (v1.type() != v2.type()) { |
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.
Using LocalDate.now() can lead to unexpected results when executed close to midnight. It should use same value as curdate.
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.
As we discussed that will be a part of another feature
@@ -31,6 +32,9 @@ | |||
}, | |||
"nested_value": { | |||
"type": "nested" | |||
}, | |||
"geo_point_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.
What does geo_point have to do with type comparison?
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 added geo_point
to have all supported types listed. No tests for this type yet.
.../test/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperatorTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/data/model/ExprTupleValue.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperator.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/data/model/ExprCollectionValue.java
Outdated
Show resolved
Hide resolved
To fix:
|
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
…orresponding tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
…` comparison. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
…anges and tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
e25b93f
to
d92fb42
Compare
Rebased and updated. Most recent change include proper use of
|
} | ||
if ((this.isNumber() && other.isNumber()) || this.type() == other.type()) { | ||
if ((this.isNumber() && other.isNumber()) |
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.
would it be better to include a isComparable(AbstractExprValue)
function here? Overridden by child classes that return true for valid cases or are the same type?
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.
If we add a new type, we have to update that function override in all/some other types to reflect the change. It is not scalable solution.
implWithProperties( | ||
SerializableTriFunction<FunctionProperties, ExprValue, ExprValue, ExprValue> function, | ||
ExprType returnType, | ||
ExprType args1Type, |
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.
better to use a List<ExprType> argTypes
instead of having to create a separate signature for each permutation
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.
Could be a separate feature. We have to change all impl
/implWithProperties
and hundreds of their usages in that case.
You are welcome to open a feature request!
return (functionProperties, v1, v2) -> { | ||
if (v1.isMissing() || v2.isMissing()) { | ||
return ExprValueUtils.missingValue(); | ||
} else if (v1.isNull() || v2.isNull()) { |
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.
else
unnecessary here
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.
Just copy-pasted from
opensearch-project-sql/core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java
Lines 320 to 331 in d92fb42
public static SerializableBiFunction<ExprValue, ExprValue, ExprValue> nullMissingHandling( | |
SerializableBiFunction<ExprValue, ExprValue, ExprValue> function) { | |
return (v1, v2) -> { | |
if (v1.isMissing() || v2.isMissing()) { | |
return ExprValueUtils.missingValue(); | |
} else if (v1.isNull() || v2.isNull()) { | |
return ExprValueUtils.nullValue(); | |
} else { | |
return function.apply(v1, v2); | |
} | |
}; | |
} |
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 can groom all of them in scope of another task I think.
core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java
Show resolved
Hide resolved
case DOUBLE: return getDoubleValue(v1).compareTo(getDoubleValue(v2)); | ||
case STRING: return getStringValue(v1).compareTo(getStringValue(v2)); | ||
case BOOLEAN: return v1.booleanValue().compareTo(v2.booleanValue()); | ||
case TIME: return v1.timeValue().compareTo(v2.timeValue()); |
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.
For time, shouldn't we potentially use datetimeValue() if the other type is DATETIME or timestampValue if the other type is TIMESTAMP?
same for date...
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 function has
v1
andv2
values of the same type, so we can compare time to time without overhead of producing and comparting timestamps. - We can't get timestamp/datetime from time without
dark magicFunctionProperties
* Extend comparison methods to accept different datetime types. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
* Extend comparison methods to accept different datetime types. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
…pensearch-project#1196) * Extend comparison methods to accept different datetime types. (#129) * Extend comparison methods to accept different datetime types. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Rework fix according to @dai-chen's comment. opensearch-project#1196 (review) * Revert `BinaryPredicateOperator`. * Add automatic cast rules `DATE`/`TIME`/`DATETIME` -> `DATETIME`/TIMESTAMP`. * Update unit tests. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Add doctest sample. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Docs typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Minor comments update. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Doctest typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Doctest typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Doctest typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Modify `ExprCoreType` dependencies. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
…pensearch-project#1196) (opensearch-project#1294) * Extend comparison methods to accept different datetime types. (#129) * Extend comparison methods to accept different datetime types. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Rework fix according to @dai-chen's comment. opensearch-project#1196 (review) * Revert `BinaryPredicateOperator`. * Add automatic cast rules `DATE`/`TIME`/`DATETIME` -> `DATETIME`/TIMESTAMP`. * Update unit tests. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Add doctest sample. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Docs typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Minor comments update. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Doctest typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Doctest typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Doctest typo fix. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Modify `ExprCoreType` dependencies. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> (cherry picked from commit a4f8066) Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand yuryf@bitquilltech.com
Description
Functions affected:
=
,!=
,>
,<
,<=
>=
.Signatures added:
Test data
Test queries
MySQL
OpenSearch
Issues Resolved
Fix comparison between different datetime data types. #294
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.