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

WIP partial parsing support #46242

Closed
wants to merge 11 commits into from

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Sep 2, 2019

strict length parsing compatible with joda
partial parsing support

#closes #45284

strict length parsing compatible with joda
@pgomulka pgomulka added WIP :Core/Infra/Core Core issues without another label labels Sep 2, 2019
@pgomulka pgomulka self-assigned this Sep 2, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka
Copy link
Contributor Author

pgomulka commented Sep 4, 2019

relaxing the date formatters uncovered the problem with combine patterns in roundUpBuilder. It is esentially the same thing as a fix we did for combined patterns without rounding up
https://bugs.openjdk.java.net/browse/JDK-8188771
JavaDateMathParserTests.testOverridingLocaleOrZoneAndCompositeRoundUpParser

pgomulka added a commit that referenced this pull request Sep 27, 2019
…#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 #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 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
@pgomulka
Copy link
Contributor Author

pgomulka commented Oct 8, 2019

closing in favour of
#46814

@pgomulka pgomulka closed this Oct 8, 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
:Core/Infra/Core Core issues without another label WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date field does not support partial dates anymore
2 participants