Core: Revert back to joda's multi date formatters #36814
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit partially reverts elastic#36447 by using the ability of Joda time's DateTimeFormatterBuilder to append multiple parsers instead of using the MergedDateFormatter. The MergedDateFormatter will be removed in a future change, as it is not as performant due to creating potentially many exceptions during heavy date parsing. This change is a stop-gap until that followup is ready. closes elastic#36602
rjernst
added
>bug
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
|
there is also an open issue in 6.x regarding exception handling that needs to be properly handled by this to retain BWC #36598 |
spinscale
suggested changes
Dec 19, 2018
| formatter = Joda.forPattern(pattern); | ||
| } | ||
| formatters.add(formatter); | ||
| formatters.add(DateFormatters.forPattern(pattern.substring(1))); |
There was a problem hiding this comment.
this needs a check for each of the pattern if it starts with 8 in order to return the proper joda time pattern for sth like 8date_optional_time||date_optional_time
There was a problem hiding this comment.
Oops, this is leftover. We won't be able to have mixed joda/java formats, so I think only having 8 as the very first character is sufficient (otherwise it would be redundant). I'll move the substring to outside the loop.
|
my suggestion to fix is this one spinscale@6b7b130 (there is one last case not covered yet, namely |
spinscale
approved these changes
Dec 19, 2018
rjernst
added a commit
that referenced
this pull request
Dec 19, 2018
This commit partially reverts #36447 by using the ability of Joda time's DateTimeFormatterBuilder to append multiple parsers instead of using the MergedDateFormatter. The MergedDateFormatter will be removed in a future change, as it is not as performant due to creating potentially many exceptions during heavy date parsing. This change is a stop-gap until that followup is ready. closes #36602
rjernst
added a commit
that referenced
this pull request
Dec 19, 2018
This commit partially reverts #36447 by using the ability of Joda time's DateTimeFormatterBuilder to append multiple parsers instead of using the MergedDateFormatter. The MergedDateFormatter will be removed in a future change, as it is not as performant due to creating potentially many exceptions during heavy date parsing. This change is a stop-gap until that followup is ready. closes #36602
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit partially reverts #36447 by using the ability of Joda time's
DateTimeFormatterBuilder to append multiple parsers instead of using the
MergedDateFormatter. The MergedDateFormatter will be removed in a future
change, as it is not as performant due to creating potentially many
exceptions during heavy date parsing. This change is a stop-gap until
that followup is ready.
closes #36602