-
Notifications
You must be signed in to change notification settings - Fork 158
Fix issue 2489 #3442
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 issue 2489 #3442
Conversation
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
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.
Remove "2015-01-01 12:10:30" related UT, it is not valid OpenSearch default date format.
This should be a breaking change that replacing DATE_TIME_FORMATTER
with DateFieldMapper.getDefaultDateTimeFormatter()
, which means we no longer support parsing value with format "yyyy-MM-dd HH:mm:ss". Shall we add this change in this issue: #3248 or somewhere else for recording breaking change after 3.0 release?
And shall we keep the format align in the final results? After this change, the input value will require format with "yyyy-MM-ddTHH:mm:ss" or epoch_millis by default if no user-specified formats, while the PPL results will always show timestamp with format "yyyy-MM-dd HH:mm:ss"
@@ -262,9 +262,10 @@ private static ExprValue parseDateTimeString(String value, OpenSearchDateType da | |||
DateFormatters.from(STRICT_YEAR_MONTH_DAY_FORMATTER.parse(value)).toLocalDate()); | |||
default: | |||
return new ExprTimestampValue( | |||
DateFormatters.from(DATE_TIME_FORMATTER.parse(value)).toInstant()); |
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.
Shall we remove DATE_TIME_FORMATTER
from DateTimeFormatters
since it's no longer used anywhere.
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.
done. 801ca25
It is not a breaking change.
Not necessary, index mapping default format enforce on ingestion. PPL could have it own default format when present result. |
Got it, although we have such test cases before, it seems could never be happened in fact for customers because opensearch will throw error when putting such a value without customized format |
Signed-off-by: Peng Huo <[email protected]>
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.
Thanks for the changes!
Any guidance when should we add test to this new yaml test? Any bug fix?
* Fix issue 2489 Signed-off-by: Peng Huo <[email protected]> * Update comments Signed-off-by: Peng Huo <[email protected]> * Integrate with YamlRestTest Signed-off-by: Peng Huo <[email protected]> * Revert change Signed-off-by: Peng Huo <[email protected]> * Fix YamlTest Signed-off-by: Peng Huo <[email protected]> * Disable security.manager Signed-off-by: Peng Huo <[email protected]> * Remove unused code Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]> (cherry picked from commit 8134c4e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Fix issue 2489 * Update comments * Integrate with YamlRestTest * Revert change * Fix YamlTest * Disable security.manager * Remove unused code --------- (cherry picked from commit 8134c4e) Signed-off-by: Peng Huo <[email protected]> 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>
* Fix issue 2489 Signed-off-by: Peng Huo <[email protected]> * Update comments Signed-off-by: Peng Huo <[email protected]> * Integrate with YamlRestTest Signed-off-by: Peng Huo <[email protected]> * Revert change Signed-off-by: Peng Huo <[email protected]> * Fix YamlTest Signed-off-by: Peng Huo <[email protected]> * Disable security.manager Signed-off-by: Peng Huo <[email protected]> * Remove unused code Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]> Signed-off-by: xinyual <[email protected]>
Description
./gradlew integ-test:build
../gradlew integ-test:yamlRestTest
.Related Issues
#2489
Check List
--signoff
.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.