-
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
Support Alternate Date Formats #258
Support Alternate Date Formats #258
Conversation
…Search types Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
integ-test/src/test/java/org/opensearch/sql/legacy/AggregationExpressionIT.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
Show resolved
Hide resolved
|
||
public List<DateFormatter> getNamedFormatters(String formats) { | ||
return getFormatList(formats).stream().filter(f -> { | ||
try { |
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.
try... catch blocks are inefficient (compared with a boolean comparison) and shouldn't be used in loops or streams.
Much better to filter on the patterns of acceptable strings.
If there's some edge cases, you can include a try...catch around the entire stream.
public List<DateTimeFormatter> getRegularFormatters() { | ||
return getFormatList(formatString).stream().map(f -> { | ||
try { | ||
return DateTimeFormatter.ofPattern(f); |
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.
same here. avoid using try...catch within a loop or stream.
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.
Probably this is the only way to distinguish formatters.
I tried to reuse (call) some methods from OpenSearch core libs, but they are not exported.
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.
Haha I tried to do the exact same thing 😆. I agree. There probably is not another way to do this.
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
Show resolved
Hide resolved
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Codecov Report
@@ Coverage Diff @@
## integ-support-date-formats #258 +/- ##
================================================================
- Coverage 99.98% 97.17% -2.81%
- Complexity 2493 4136 +1643
================================================================
Files 193 372 +179
Lines 5720 10421 +4701
Branches 359 707 +348
================================================================
+ Hits 5719 10127 +4408
- Misses 1 287 +286
- Partials 0 7 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 172 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
… With Datetime Types Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
Outdated
Show resolved
Hide resolved
public List<DateTimeFormatter> getRegularFormatters() { | ||
return getFormatList(formatString).stream().map(f -> { | ||
try { | ||
return DateTimeFormatter.ofPattern(f); |
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.
Probably this is the only way to distinguish formatters.
I tried to reuse (call) some methods from OpenSearch core libs, but they are not exported.
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Outdated
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/OpenSearchDataTypeTest.java
Show resolved
Hide resolved
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com> (cherry picked from commit c91f48d9763499940ecf302212fcd592aecb6018)
…earch-project-sql into dev-support-date-formats
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java
Outdated
Show resolved
Hide resolved
new DateTimeFormatterBuilder() | ||
.appendOptional(SQL_LITERAL_DATE_TIME_FORMAT) | ||
.appendOptional(STRICT_DATE_OPTIONAL_TIME_FORMATTER) | ||
.appendOptional(STRICT_HOUR_MINUTE_SECOND_FORMATTER); |
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 I understand, you use 3 hardcoded formats instead of formats from mapping
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 should be fine? This is only slightly modified from how it was implemented here (I modified it as a potential way to try and use user-defined formats). However, this might not even be needed anymore. constructTimestamp
is only called in one place, if the call to parseTimestampString
fails to parse the value, so probably the call to to constructTimestamp
isn't even needed. I will try to rework.
opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java
Outdated
Show resolved
Hide resolved
…ct-sql into dev-support-date-formats
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
…ct-sql into dev-support-date-formats
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
…nsearch-project-sql into dev-support-date-formats
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
@GabeFernandez310 can you please add a use case to the PR description? If there are any 'breaking' changes, please list them in the PR description. I believe, the only breaking changes would happen if a date format was defined for a mapping which was previously unused - and now it may not resolve that date if the date doesn't match the default format AND the format provided doesn't include the default type. Honestly, the above change seems like it would be bad data anyways... so is this really a breaking change? |
@@ -483,6 +484,7 @@ public void joinQuerySelectOnlyOnOneTable() throws Exception { | |||
assertContainsData(getDataRows(response), fields); | |||
} | |||
|
|||
@Disabled("Disabled temporarily due to JSON format incompatibility with V2 and Legacy") |
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 do you mean by this? Is this test case now supported in V2 and it fails?
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 may have been a mistake. I think this was failing due to the data type refactor at first, and when I asked about it JSON format was the reason that was suggested for why it started failing all of a sudden, so I just put that as the description. This has been fixed since then, so I will remove these annotations.
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.
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
* Keep up with refactoring in OpenSearch. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> * Updating code formatting. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
{"index": {}} | ||
{"name": "yyyy-MM-dd||uuuu-DDD", "yyyy-MM-dd||uuuu-DDD": "1984-04-12"} | ||
{"index": {}} | ||
{"name": "hour_minute_second||t_time", "hour_minute_second||t_time": "09:07:42"} |
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 we add multi-format field that covers both dates and times?
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.
{"index": {}} | ||
{"name": "HH:mm:ss", "HH:mm:ss": "09:07:42"} | ||
{"index": {}} | ||
{"name": "yyyy-MM-dd||uuuu-DDD", "yyyy-MM-dd||uuuu-DDD": "1984-04-12"} |
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 should have data that formats to each of the provided date formats to make sure that they are all working.
ie add data that conforms to uuuu-DDD
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.
{"index": {}} | ||
{"name": "yyyy-MM-dd||uuuu-DDD", "yyyy-MM-dd||uuuu-DDD": "1984-04-12"} | ||
{"index": {}} | ||
{"name": "hour_minute_second||t_time", "hour_minute_second||t_time": "09:07:42"} |
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 add data that is formatted by the t_time
formatter
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.
*/ | ||
public static Map<String, OpenSearchDataType> parseMapping(Map<String, Object> indexMapping) { | ||
Map<String, OpenSearchDataType> result = new LinkedHashMap<>(); | ||
if (indexMapping != 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.
nit: I prefer to fail early on functions. e.g.
if (indexMapping == null) {
return result;
}
...
switch (mappingType) { | ||
case Object: | ||
case Nested: | ||
if (innerMap.isEmpty()) { |
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 wonder if this should be applied prior to the switch
as really any innerMap that's empty should just return the cached instance.
return new ExprTimestampValue(Instant.ofEpochMilli(value.longValue())); | ||
return formatReturn( | ||
returnFormat, | ||
new ExprTimestampValue(Instant.ofEpochMilli(value.longValue()))); | ||
} else if (value.isString()) { |
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
not required
"Construct ExprTimestampValue from \"%s\" failed, unsupported date format.", | ||
value.stringValue()), | ||
ignored); | ||
} | ||
} else { |
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
not required, as the other options all return/throw
dt = (OpenSearchDateType) type; | ||
returnFormat = dt.getExprType(); | ||
} else { | ||
dt = OpenSearchDateType.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.
Why would this happen? I mean, when would we parseTimestamp on a non-date 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.
Probably needs to be reworked. Currently typeActionMap
still uses an OpenSearchDataType
created from an ExprCoreType
in some places. I believe I tried to change this at some point, but was seeing test failures because in the case that an OpenSearchDataType
is passed in, it was failing on the line
dt = (OpenSearchDateType) type;
because it could not cast the passed in type
argument to an OpenSearchDateType
.
This if-else
branch may just need to be removed, and the code may need to be updated wherever the parseTimestamp
function is called using a type
argument that is not an OpenSearchDateType
.
opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java
Show resolved
Hide resolved
@@ -28,6 +28,7 @@ | |||
import java.util.Map; | |||
import lombok.RequiredArgsConstructor; | |||
import org.apache.lucene.index.LeafReaderContext; | |||
import org.junit.Ignore; |
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.
revert this?
private static final String FORMAT_DELIMITER = "\\|\\|"; | ||
|
||
|
||
// a read-only collection of relations |
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.
update comment
* Of type join with relations. See | ||
* <a href="https://opensearch.org/docs/latest/opensearch/supported-field-types/join/">doc</a> |
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.
Update comment
opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
Show resolved
Hide resolved
var res = new OpenSearchDateType(format); | ||
return res; |
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 return new ...
returnFormat, | ||
new ExprTimestampValue( | ||
new ExprTimeValue(LocalTime.from(parsed)) | ||
.timestampValue(new FunctionProperties(Instant.EPOCH, ZoneOffset.UTC)))); |
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 works as LocalDate.now()
I suppose.
We need a way to use here already created for this query instance of FunctionProperties
.
… dev-support-date-formats
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
…earch-project-sql into dev-support-date-formats
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Closing this as there will be a follow up PR to isolate the support for datetime formats only. |
Description
Supports using date formats obtained from OpenSearch. Contains some changes from OpenSearchDataType rework (8b2f65d). Resolves some issues previously seen on the forum (please see Issues Resolved section below). This also adds support to use custom formats in the index mapping. Valid custom formats must have enough information to be parsed as a Date, Time, or Timestamp, and otherwise throw an exception after failing to parse.
Currently failing due to test failures likely unrelated to these specific changes
Based on PoC (Here: https://github.com/Bit-Quill/opensearch-project-sql/pull/169/files). Therefore, it also fails to parse the same formats as those listed in the PoC.
Note: With this change, issues may be introduced where data is read differently if a user's mapping defines a format. This is because the plugin previously used a hardcoded formatter to parse the data, while with these changes it will use the formatter defined in the mapping.
Supported formats include the following:
The following formats are not supported as no default formatters are available for these from OpenSearch core.
Issues Resolved
opensearch-project#794
https://forum.opensearch.org/t/sql-select-fails-on-date-fields-format-epoch-second/11521
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.