-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Date/Time parsing: Use java time API instead of exception handling #37222
Date/Time parsing: Use java time API instead of exception handling #37222
Conversation
when several formatters are used, the existing way of parsing those is to throw an exception catch it, and try the next one. This is is considerably slower than the approach taken in joda time, so that indexing is reduced when a date format like `x||y` is used and y is the date format being used. This commit now uses the java API to parse the date by appending the date time formatters to each other and does not rely on exception handling.
Pinging @elastic/es-core-infra |
@elasticmachine please retest this |
this requires more investigation, parsing seems to be broken, when using more than one parser, only the first one is applied correctly |
@elasticmachine retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine overall, I left a few minor comments.
benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java
Outdated
Show resolved
Hide resolved
benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
Outdated
Show resolved
Hide resolved
thanks! applied your comments |
@elasticmachine retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating. Can you please rerun the microbenchmarks with the new changes and update the description? LGTM if Jenkins is also happy.
there was no significant change in the benchmark stats, thus I didnt update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…37222) when several formatters are used, the existing way of parsing those is to throw an exception, catch it, and try the next one. This is is considerably slower than the approach taken in joda time, so that indexing throughput is reduced when a date format like `x||y` is used and y is the date format being used. This commit now uses the java API to parse the date by appending the date time formatters to each other and does not rely on exception handling.
when several formatters are used, the existing way of parsing those is
to throw an exception catch it, and try the next one. This is is
considerably slower than the approach taken in joda time, so that
indexing throughput is reduced when a date format like
x||y
is used and y is thedate format being used.
This commit now uses the java API to parse the date by appending the
date time formatters to each other and does not rely on exception
handling.
This also removes the
MergedDateFormatter
class, is this kind ofwork can now be done before the class is created.
The stats of the added benchmark before this patch
with this patch
I also played around a bit with the benchmark using
parseJoda
for joda, but there are no real runtime changes.