-
Notifications
You must be signed in to change notification settings - Fork 139
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
Fix to address bug in the SQL query with timestamp field format #2765
Conversation
looks like CI failed due to spotless violations. |
Signed-off-by: Manasvini B S <manasvis@amazon.com>
b27e895
to
ce96609
Compare
@@ -1821,7 +1821,7 @@ void cast_to_timestamp_in_filter() { | |||
"{\n" | |||
+ " \"term\" : {\n" | |||
+ " \"timestamp_value\" : {\n" | |||
+ " \"value\" : 1636390800000,\n" | |||
+ " \"value\" : \"2021-11-08 17:00:00\",\n" |
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 add test with different format? We should support all formats mentioned in https://opensearch.org/docs/latest/field-types/supported-field-types/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.
Can you also add IT to cover this case? IIRC we don't have IT with date field in where clause.
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 is the issue with epoch value? why we want cast to ISO 8601 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.
My intention with this PR was to remove additional formatting to specifically epoch on literal value during query build. If we expect literal value to be the right formatted value, then this additional default formatting can cause issues.
Having said that today these literal values today are not honoring all the right date/time formats. See the formatter we are using today to format timestamp string in ExprTimeStamp.value class
LocalDateTime.parse(timestamp, DATE_TIME_FORMATTER_VARIABLE_NANOS) |
Its always yyyy-MM-dd HH:mm:ss
for now
.appendPattern("uuuu-MM-dd HH:mm:ss") |
So in this case literal value is ISO 8601. But if we fix the format while parsing the date string in ExprTimeStampValue/ExprDateValue before using it in query builders, we do not have to format to epoch or any other default values.
Thanks for catching. Fixed and updated. |
Closing this in context of this PR #2762 as this PR addresses the larger issue with custom date formats and opensearch date formats support for lucene queries. |
Description
We have this issue in SQL query when date in timestamp format in the SQL query is erroring out due to mismatch in the timestamp format between field mapping and the request query -
Sample query:
Error -
Root cause -
Currently for response JDBC format, when term/range query has timestamp field in the sql query, by default we are converting timestamps to epoch time in milliseconds when building query for Opensearch DSL which is causing the above exceptions. Instead for timestamp query string in epoch, we should send parsed formatted timestamp string as it is as part of the query similar to Date format.
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.