-
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
Add support for custom date formats and OpenSearch date formats for date fields as part of Lucene query #2762
Conversation
|
||
/** Constructor of ExprDateValue. */ | ||
public ExprDateValue(String date) { | ||
try { | ||
this.date = LocalDate.parse(date, DATE_TIME_FORMATTER_VARIABLE_NANOS_OPTIONAL); | ||
this.datePattern = determineDatePattern(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.
does parese execute 2 times, the reasons is LocalDate.parse already parse the date string
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.
On line #44 here, yes we are already parsing the date string to Local date object. But I need to know the exact pattern that matched the input date string. So inside determineDatePattern
function, using parse
function (LocalDate/LocalDatetime) I'm determining the matching pattern and returning that to set this.datePattern
. This date pattern is used to format the LocalDate date object instead of default ISO_LOCAL_DATE string inside value()
.
core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/data/model/DateTimeValueTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.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.
Seems you need to reformat the code using ./gradlew spotlessApply
.
core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java
Outdated
Show resolved
Hide resolved
a2c9f68
to
c49231d
Compare
...ch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java
Outdated
Show resolved
Hide resolved
...ch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java
Outdated
Show resolved
Hide resolved
...rch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQuery.java
Outdated
Show resolved
Hide resolved
...rch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQuery.java
Outdated
Show resolved
Hide resolved
List<DateFormatter> dateFormatters = this.getAllNamedFormatters(); | ||
dateFormatters.addAll(this.getAllCustomFormatters()); | ||
ZonedDateTime zonedDateTime = null; | ||
|
||
// check if dateFormatters are empty, then set default ones | ||
if (dateFormatters.isEmpty()) { | ||
dateFormatters = initializeDateFormatters(); | ||
} |
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.
List<DateFormatter> dateFormatters = this.getAllNamedFormatters();
dateFormatters.addAll(this.getAllCustomFormatters());
this code will duplicate getAllCustomFormatters?
use Stream.concat to concat two list works?
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.
formats is fetched from OpenSearch index mapping, if formats is null, does it means the OpenSearchDataType is a wrapper of CoreType. So in this case, should we use OPENSEARCH_DEFAULT_FORMATS to format to parse the data?
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.
When format is null, it can still be OpenSearchDateType
field as there can be date fields without formats (default ones) defined. So I have used OPENSEARCH_DEFAULT_FORMATS to format the date object in those cases.
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
Show resolved
Hide resolved
opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateTypeTest.java
Show resolved
Hide resolved
...ch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java
Outdated
Show resolved
Hide resolved
…e fields as part of Lucene query Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com>
@@ -400,6 +400,48 @@ Querying such index will provide a response with ``schema`` block as shown below | |||
"status": 200 | |||
} | |||
|
|||
If the sql query contains an `IndexDateField` and a literal value with an operator (such as a term query or a range query), then the literal value can be in the `IndexDateField` 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.
term query and range query is OpenSearch concept, there is not function / operator in SQL is term/range.
@@ -230,6 +234,9 @@ public String legacyTypeName() { | |||
if (mappingType == null) { | |||
return exprCoreType.typeName(); | |||
} | |||
if (mappingType.toString().equalsIgnoreCase("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.
change to mappingType == 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.
Should we check mappingType == DateNanos either?
if (mappingType == Date || mappingType == DateNanos) {
@@ -104,8 +104,8 @@ private static Stream<Arguments> getTestDataWithType() { | |||
Arguments.of(MappingType.ScaledFloat, "scaled_float", DOUBLE), | |||
Arguments.of(MappingType.Double, "double", DOUBLE), | |||
Arguments.of(MappingType.Boolean, "boolean", BOOLEAN), | |||
Arguments.of(MappingType.Date, "date", TIMESTAMP), | |||
Arguments.of(MappingType.DateNanos, "date", TIMESTAMP), | |||
Arguments.of(MappingType.Date, "timestamp", TIMESTAMP), |
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 double confirm on UT chagne. does all these date -> timestamp change necessary?
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 these assertions directly comparing with legacyTypeName
and hence need this update. I think these changes are necessary and only in UT to avoid any breaking changes with type change from ExprCoreType
to OpenSearchDateType
inside fieldMappings
map.
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 only find the legacy name change for Date
in production code, why the DateNanos
changed in test either? https://github.com/opensearch-project/sql/pull/2762/files/ea3e205184dda4e8e0d5ba55cd9cf18eac82c30e#r1683753172
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 think this is because when we create OpenSearchDataType
instance for every field mapping type, we are creating OpenSearchDateType
instance for both Mappingtype.Date
and MappingType.DateNanos
- https://github.com/opensearch-project/sql/blob/main/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java#L165
and set the mapping type to Date even for DateNanos (not sure why is it this way) - https://github.com/opensearch-project/sql/blob/main/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java#L143
So I think here UT assertion still looking for Mapping type date even for date nanos I believe.
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 double confirm on the UT assert change,
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-2762-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0fad56db4b3e8983e2e7fafcf9fb80e592d97ddb
# Push it to GitHub
git push --set-upstream origin backport/backport-2762-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x Then, create a pull request where the |
…e fields as part of Lucene query (opensearch-project#2762) Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com> (cherry picked from commit 0fad56d)
…e fields as part of Lucene query (opensearch-project#2762) Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com> (cherry picked from commit 0fad56d) Signed-off-by: Manasvini B S <manasvis@amazon.com>
…e fields as part of Lucene query (opensearch-project#2762) Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com> (cherry picked from commit 0fad56d) Signed-off-by: Manasvini B S <manasvis@amazon.com>
…e fields as part of Lucene query (opensearch-project#2762) Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com> (cherry picked from commit 0fad56d) Signed-off-by: Manasvini B S <manasvis@amazon.com>
…e fields as part of Lucene query (#2762) (#2849) (#2851) Github Issue - #2700 (cherry picked from commit 0fad56d) (cherry picked from commit 02d57e0) Signed-off-by: Manasvini B S <manasvis@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…e fields as part of Lucene query (opensearch-project#2762) Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com>
…e fields as part of Lucene query (opensearch-project#2762) Github Issue - opensearch-project#2700 Signed-off-by: Manasvini B S <manasvis@amazon.com>
Description
This change adds support for OpenSearch Date formats and custom date formats to be part of SQL Lucene queries.
Without this change, we are supporting only list of selected formats which always gets formatted to ISO local string or epoch before submitting to opensearch dsl query.
ExprType
to beOpenSearchDateType
object instead ofExprCoretype
enums passed as a param to the Lucene QueryBuilder.OpenSearchDateNamedFormatters
andOpenSearchDateCustomFormatters
from theOpenSearchDateType
object which is set during IndexMapping field parsing for a field type.ExprValue
implementation constructor to parse the date string using the field specific formatters.OpenSearch
package dependency to thecore
module to use OpenSearch providedDateFormatter
andDateFormatters
classes instead of java provided DateFormatter.Follow-up PR - I'll add IT tests in a separate PR
Issues Resolved
#2700
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.