Parse composite patterns using ClassicFormat.parseObject #40100

pgomulka · 2019-03-15T14:55:05Z

Java-time fails parsing composite patterns when first pattern matches
only the prefix of the input. It expects pattern in longest-shortest
order.
Joda does not suffer from this

Closes #39916

Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest-shortest order. Joda does not suffer from this closes elastic#39916

elasticmachine · 2019-03-15T14:55:07Z

Pinging @elastic/es-core-infra

pgomulka · 2019-03-15T20:30:50Z

@elasticmachine run elasticsearch-ci/packaging-sample

pgomulka · 2019-03-18T10:59:38Z

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

@@ -1585,7 +1585,7 @@ static JavaDateFormatter merge(String pattern, List<DateFormatter> formatters) {
            if (printer == null) {
                printer = javaDateFormatter.getPrinter();
            }
-            dateTimeFormatters.add(javaDateFormatter.getParser());
+            dateTimeFormatters.addAll(javaDateFormatter.getParsers());


not a fan of this merge method. It extracts relatively low level java.time.DateTimeFormatter when it should stick with org.elasticsearch.common.time.DateFormatter abstraction as long as possible.
Possibly the roundUpBuilder should also be used inside JavaDateFormatter constructor?

also I suspect that roundUpBuilder will suffer from the same problem when it is a composite?

not a fan of this merge method. It extracts relatively low level java.time.DateTimeFormatter when it should stick with org.elasticsearch.common.time.DateFormatter abstraction as long as possible.

Maybe we can refactor this in a separate PR then? Also, this code should only be there temporary? As soon as we get rid of Joda time in the code base, I expect that we can get rid of quite a few abstractions.

I suspect that roundUpBuilder will suffer from the same problem when it is a composite?

Is it intended to be used as a composite?

Is it intended to be used as a composite?

It is used as a composite in the merge method when constructing the roundUpParser with the appendOptional.
I can imagine we can have the same pattern as on the issue (yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS) but used with this parser. Will try to come up with a testcase

I discussed that with @spinscale and there is no way we could create a pattern that would suffer from the same problem.
For index name calculations like prefix-{2010-01-01/d{yyyy-MM-dd||yyyy-MM-ddTHH}} it would fail parsing, as the only thing expected after | is the timezone (it would fail saying unexpected |)

pgomulka · 2019-03-18T13:45:04Z

this change is a bit slower.

on my laptop on this PR branch:

# Run complete. Total time: 00:02:05

Benchmark                             Mode  Cnt     Score    Error  Units
DateFormatterBenchmark.parseJavaDate  avgt   30   923.107 ± 31.714  ns/op
DateFormatterBenchmark.parseJodaDate  avgt   30  1001.870 ± 29.654  ns/op

vs on master

# Run complete. Total time: 00:02:05

Benchmark                             Mode  Cnt     Score    Error  Units
DateFormatterBenchmark.parseJavaDate  avgt   30   807.209 ± 10.100  ns/op
DateFormatterBenchmark.parseJodaDate  avgt   30  1023.225 ± 61.066  ns/op

danielmitterdorfer

I left a couple of suggestions.

server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java

danielmitterdorfer · 2019-03-18T16:02:39Z

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

@@ -1585,7 +1585,7 @@ static JavaDateFormatter merge(String pattern, List<DateFormatter> formatters) {
            if (printer == null) {
                printer = javaDateFormatter.getPrinter();
            }
-            dateTimeFormatters.add(javaDateFormatter.getParser());
+            dateTimeFormatters.addAll(javaDateFormatter.getParsers());


not a fan of this merge method. It extracts relatively low level java.time.DateTimeFormatter when it should stick with org.elasticsearch.common.time.DateFormatter abstraction as long as possible.

Maybe we can refactor this in a separate PR then? Also, this code should only be there temporary? As soon as we get rid of Joda time in the code base, I expect that we can get rid of quite a few abstractions.

I suspect that roundUpBuilder will suffer from the same problem when it is a composite?

Is it intended to be used as a composite?

server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java

danielmitterdorfer · 2019-03-18T16:13:36Z

server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java

    }

    @Override
    public DateFormatter withLocale(Locale locale) {
+        //TODO not sure that we can keep that shortcut, and the one above


probably not because previously we had a common locale for the composite parser and now it is possible that each one has a different locale (not sure whether we should allow this though and throw an IllegalArgumentException in the constructor if that it is the case?) Also, we should probably still keep this optimization in case we only have one parser I think

agree. I don't think we have a code were we define composite formatters with different Locale
We could also refactor this into verifying all parsers having the same Locale or Zone before returning this.

server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java

spinscale · 2019-03-19T10:44:58Z

server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java

+                }
+            }
+        }
+        return firstParser().parse(input);


is this doing a second parse attempt with the first parser, if going through all the parsers did not work?

yes, I guess we could throw an exception after the loop. WDYT?

or just be sure via ctor check, that the size is never 0? maybe just use an array instead of an array list, than the below check would always be a fixed size check as well?

It should never be 0, if no parsers are provided, the printer will be used.
The only reason for this >1 check is to at least when there is only one parser (the regular, not composite pattern) it is not parsing the input twice.

as discussed, will add an exception indicating that all parsers has failed. This won't mislead that the first parser failed (as it is at the moment)

Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest to shortest order. Because of this constructing just one DateTimeFormatter with appendOptional is not sufficient. Parsers have to be iterated and if the parsing fails, the next one in order should be used. In order to not degrade performance parsing should not be throw exceptions on failure. Format.parseObject was used as it only returns null when parsing failed and allows to check if full input was read. closes elastic#39916

) (#40501) Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest to shortest order. Because of this constructing just one DateTimeFormatter with appendOptional is not sufficient. Parsers have to be iterated and if the parsing fails, the next one in order should be used. In order to not degrade performance parsing should not be throw exceptions on failure. Format.parseObject was used as it only returns null when parsing failed and allows to check if full input was read. closes #39916 backport #40100

) (#40502) Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest to shortest order. Because of this constructing just one DateTimeFormatter with appendOptional is not sufficient. Parsers have to be iterated and if the parsing fails, the next one in order should be used. In order to not degrade performance parsing should not be throw exceptions on failure. Format.parseObject was used as it only returns null when parsing failed and allows to check if full input was read. closes #39916 backport #40100

rjernst

Sorry for the late review here. I just have one ask for ensuring this works in all cases.

rjernst · 2019-03-28T18:04:58Z

server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java

@@ -343,6 +343,17 @@ public void testDuellingFormatsValidParsing() {
        assertSameDate("2012-W1-1", "weekyear_week_day");
    }

+    public void testCompositeParsing(){
+        //in all these examples the second pattern will be used
+        assertSameDate("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS");


I think we need some more tests:

3 patterns

random ordering of many patterns

Happy to add more test cases. Do you have anything specific in mind?
It won't harm adding testcases with 3 patterns, but I am not convinced we need a random ordering here. What scenario would it cover?

) (#40503) Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest to shortest order. Because of this constructing just one DateTimeFormatter with appendOptional is not sufficient. Parsers have to be iterated and if the parsing fails, the next one in order should be used. In order to not degrade performance parsing should not be throw exceptions on failure. Format.parseObject was used as it only returns null when parsing failed and allows to check if full input was read. closes #39916 backport #40100

…#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

…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

…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

…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

Parse patterns with optionals using parseUnresolved

97d0472

Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest-shortest order. Joda does not suffer from this closes elastic#39916

pgomulka added >bug :Core/Infra/Core Core issues without another label v7.0.0 v8.0.0 v7.2.0 labels Mar 15, 2019

pgomulka self-assigned this Mar 15, 2019

pgomulka requested review from rjernst, spinscale and polyfractal March 15, 2019 14:55

unused imports

4c7e997

pgomulka changed the title ~~Parse patterns with optionals using parseUnresolved~~ Parse composite patterns using parseUnresolved Mar 15, 2019

pgomulka added 5 commits March 15, 2019 16:29

exception formatting

a6448d2

parsing pass

aa505e2

refactor

abb0bc6

tab

5bcbb7b

revert

44ae404

pgomulka added 2 commits March 17, 2019 18:51

Merge branch 'master' into feature/parse_pattern_optional

945a6ad

javadatemath parser to work with merged parser

68b11f4

pgomulka commented Mar 18, 2019

View reviewed changes

pgomulka added 3 commits March 18, 2019 12:21

codestyle

8fb210c

passing tests

0f2fca3

unused import

22269ce

Merge branch 'master' into feature/parse_pattern_optional

71c149e

danielmitterdorfer reviewed Mar 18, 2019

View reviewed changes

code review follow up

ef01371

spinscale reviewed Mar 19, 2019

View reviewed changes

pgomulka mentioned this pull request Mar 27, 2019

Parse composite patterns using ClassicFormat.parseObject backport(#40100) #40502

Merged

pgomulka mentioned this pull request Mar 27, 2019

Parse composite patterns using ClassicFormat.parseObject backport(#40100) #40503

Merged

rjernst reviewed Mar 28, 2019

View reviewed changes

jakelandis added v7.0.0-rc2 and removed v7.0.0 labels Apr 3, 2019

pgomulka mentioned this pull request Jun 5, 2019

Broken range queries on "date||epoch_millis" fields in 7.1 #42835

Closed

jakelandis added v7.3.0 and removed v6.7.1 v7.0.0-rc2 v7.2.0 labels Jun 17, 2019

colings86 added the v7.2.0 label Jun 18, 2019

jpountz removed backport pending v7.3.0 labels Jul 5, 2019

pgomulka mentioned this pull request Sep 12, 2019

Support optional parsers in any order with DateMathParser and roundup #46654

Merged

pgomulka mentioned this pull request Sep 27, 2019

Support optional parsers in any order with DateMathParser Backport(46654) #47217

Merged

pgomulka added the v6.7.3 label May 20, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse composite patterns using ClassicFormat.parseObject #40100

Parse composite patterns using ClassicFormat.parseObject #40100

pgomulka commented Mar 15, 2019 •

edited by colings86

Loading

elasticmachine commented Mar 15, 2019

pgomulka commented Mar 15, 2019

pgomulka Mar 18, 2019 •

edited

Loading

danielmitterdorfer Mar 18, 2019

pgomulka Mar 18, 2019

pgomulka Mar 19, 2019

pgomulka commented Mar 18, 2019 •

edited

Loading

danielmitterdorfer left a comment

danielmitterdorfer Mar 18, 2019

danielmitterdorfer Mar 18, 2019

pgomulka Mar 19, 2019

spinscale Mar 19, 2019

pgomulka Mar 19, 2019

spinscale Mar 19, 2019

pgomulka Mar 19, 2019

pgomulka Mar 19, 2019

rjernst left a comment

rjernst Mar 28, 2019

pgomulka Mar 29, 2019

Parse composite patterns using ClassicFormat.parseObject #40100

Parse composite patterns using ClassicFormat.parseObject #40100

Conversation

pgomulka commented Mar 15, 2019 • edited by colings86 Loading

elasticmachine commented Mar 15, 2019

pgomulka commented Mar 15, 2019

pgomulka Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka commented Mar 18, 2019 • edited Loading

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka commented Mar 15, 2019 •

edited by colings86

Loading

pgomulka Mar 18, 2019 •

edited

Loading

pgomulka commented Mar 18, 2019 •

edited

Loading