Skip to content

Conversation

@rjernst
Copy link
Member

@rjernst rjernst commented Dec 20, 2018

This commit converts the epoch time parsing implementation which uses
the java time api to create DateTimeFormatters instead of DateFormatter
implementations. This will allow multi formats for java time to be
implemented in a single DateTimeFormatter in a future change.

This commit converts the epoch time parsing implementation which uses
the java time api to create DateTimeFormatters instead of DateFormatter
implementations. This will allow multi formats for java time to be
implemented in a single DateTimeFormatter in a future change.
@rjernst rjernst added :Core/Infra/Core Core issues without another label v7.0.0 >refactoring v6.7.0 labels Dec 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@rjernst rjernst requested a review from spinscale December 20, 2018 21:37
Copy link
Contributor

@spinscale spinscale left a comment

Choose a reason for hiding this comment

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

LGTM in general, I am not sure about the test case failure yet.

I could not reproduce the test failure in my IDE using just java11, but only via gradle / maybe there is some bwc issue with older java versions...

@rjernst
Copy link
Member Author

rjernst commented Dec 22, 2018

I tracked the failure down to a bug in java 8 (the test passes in java 11). According to the javadocs of DateTimeFormatterBuilder, optional sections should only be output when formatting if they are supported. When using our custom TemporalField in java 8, the isSupportedBy never gets called on our nanos field, it is simply retrieved and always output.

I can think about how to workaround this, but I believe it will be difficult and require a change higher up to detect this and choose a different formatter to use. Given that this is fixed in java 11, I am inclined to say this is simply a known issue and amend the test to handle it. @spinscale thoughts?

@spinscale
Copy link
Contributor

@rjernst I tend to agree. The only thing this could be uncovered by the end user are names of aggs and scripts (where I am assuming that this formatting is rare).

@spinscale
Copy link
Contributor

I created a super small benchmark, which basically uses the date formatter "year_month_day||ordinal_date||epoch_millis" and then converts a simple number so that only the last formatter matches

results in the java-time branch (which does not differ too much from master right now in that regard)

Benchmark                             Mode  Cnt      Score     Error  Units
DateFormatterBenchmark.parseJavaDate  avgt   30  11586,309 ± 347,316  ns/op
DateFormatterBenchmark.parseJodaDate  avgt   30   1105,384 ±  24,877  ns/op

results with this PR

Benchmark                             Mode  Cnt     Score    Error  Units
DateFormatterBenchmark.parseJavaDate  avgt   30  1104,080 ± 14,440  ns/op
DateFormatterBenchmark.parseJodaDate  avgt   30  1117,427 ± 21,235  ns/op

@rjernst
Copy link
Member Author

rjernst commented Jan 7, 2019

@elasticmachine run gradle build tests 1

@rjernst rjernst merged commit 55d3ca3 into elastic:master Jan 8, 2019
@rjernst rjernst deleted the timeapi20 branch January 8, 2019 00:15
rjernst added a commit that referenced this pull request Jan 8, 2019
This commit converts the epoch time parsing implementation which uses
the java time api to create DateTimeFormatters instead of DateFormatter
implementations. This will allow multi formats for java time to be
implemented in a single DateTimeFormatter in a future change.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
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.

4 participants