Skip to content
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 optional parsers in any order with DateMathParser and roundup #46654

Merged
merged 17 commits into from
Sep 27, 2019

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Sep 12, 2019

Currently DateMathParser with roundUp = true is relying on the DateFormatter build with combined optional sub parsers with defaulted fields (depending on the formatter). That means that for yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS
Java.time implementation expects optional parsers in order from most specific to least specific (reverse in the example above).
It is causing a problem because the first parsing succeeds but does not consume the full input. The second parser should be used.
We can work around this with keeping a list of RoundUpParsers and iterate over them choosing the one that parsed full input. The same approach we used for regular (non date math) in
relates #40100
The jdk is not considering this to be a bug https://bugs.openjdk.java.net/browse/JDK-8188771

Those below will expect this change first
relates #46242
relates #45284

@pgomulka pgomulka added the :Core/Infra/Core Core issues without another label label Sep 12, 2019
@pgomulka pgomulka self-assigned this Sep 12, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka pgomulka changed the title Feature/refactor round up Support optional parsers in any order with DateMathParser and roundup Sep 12, 2019
@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

1 similar comment
@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

1 similar comment
@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}

private static DateTimeFormatter firstFrom(List<DateTimeFormatter> roundUpParsers) {
if(roundUpParsers != null && roundUpParsers.size()>0){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null is not possible here, since a null check was done before calling the ctor

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka pgomulka merged commit b50164f into elastic:master Sep 27, 2019
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Sep 27, 2019
…elastic#46654)

Currently DateMathParser with roundUp = true is relying on the DateFormatter build with
combined optional sub parsers with defaulted fields (depending on the formatter).
That means that for yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS
Java.time implementation expects optional parsers in order from most specific to
least specific (reverse in the example above).
It is causing a problem because the first parsing succeeds but does not consume the full input.
The second parser should be used.
We can work around this with keeping a list of RoundUpParsers and iterate over them choosing
the one that parsed full input. The same approach we used for regular (non date math) in
relates elastic#40100
The jdk is not considering this to be a bug https://bugs.openjdk.java.net/browse/JDK-8188771

Those below will expect this change first
relates elastic#46242
relates elastic#45284
pgomulka added a commit that referenced this pull request Sep 30, 2019
…654) (#47217)

Currently DateMathParser with roundUp = true is relying on the DateFormatter build with
combined optional sub parsers with defaulted fields (depending on the formatter).
That means that for yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS
Java.time implementation expects optional parsers in order from most specific to
least specific (reverse in the example above).
It is causing a problem because the first parsing succeeds but does not consume the full input.
The second parser should be used.
We can work around this with keeping a list of RoundUpParsers and iterate over them choosing
the one that parsed full input. The same approach we used for regular (non date math) in
relates #40100
The jdk is not considering this to be a bug https://bugs.openjdk.java.net/browse/JDK-8188771

Those below will expect this change first
relates #46242
relates #45284
backport #46654
@jimczi jimczi added the >bug label Nov 12, 2019
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Dec 9, 2019
…654) (elastic#47217)

Currently DateMathParser with roundUp = true is relying on the DateFormatter build with
combined optional sub parsers with defaulted fields (depending on the formatter).
That means that for yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS
Java.time implementation expects optional parsers in order from most specific to
least specific (reverse in the example above).
It is causing a problem because the first parsing succeeds but does not consume the full input.
The second parser should be used.
We can work around this with keeping a list of RoundUpParsers and iterate over them choosing
the one that parsed full input. The same approach we used for regular (non date math) in
relates elastic#40100
The jdk is not considering this to be a bug https://bugs.openjdk.java.net/browse/JDK-8188771

Those below will expect this change first
relates elastic#46242
relates elastic#45284
backport elastic#46654
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 7, 2020
…654) (elastic#47217)

Currently DateMathParser with roundUp = true is relying on the DateFormatter build with
combined optional sub parsers with defaulted fields (depending on the formatter).
That means that for yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS
Java.time implementation expects optional parsers in order from most specific to
least specific (reverse in the example above).
It is causing a problem because the first parsing succeeds but does not consume the full input.
The second parser should be used.
We can work around this with keeping a list of RoundUpParsers and iterate over them choosing
the one that parsed full input. The same approach we used for regular (non date math) in
relates elastic#40100
The jdk is not considering this to be a bug https://bugs.openjdk.java.net/browse/JDK-8188771

Those below will expect this change first
relates elastic#46242
relates elastic#45284
backport elastic#46654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants