Extend the date rounding logic to be conditional by pgomulka · Pull Request #89693 · elastic/elasticsearch

pgomulka · 2022-08-29T13:17:01Z

Date rounding logic should take into account the fields that will be
parsed be a parser. If a parser has a DayOfYear field, the rounding logic
should not try to default DayOfMonth as it will conflict with DayOfYear

However the DateTimeFormatter does not have a public method to return
information of fields that will be parsed. The hacky workaround is
to rely on toString() implementation that will return a field info when
it was defined with textual pattern.

This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM

closes #89096
closes #58986

Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes elastic#89096 closes elastic#58986

elasticsearchmachine · 2022-08-29T13:17:25Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2022-08-29T13:17:25Z

Hi @pgomulka, I've created a changelog YAML for you.

grcevski

LGTM! I left one small remark about potential performance issue.

grcevski · 2022-08-29T14:26:21Z

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

+    * without this logic, the rounding would result in a conflict as HOUR_OF_DAY would be missing, but CLOCK_HOUR_OF_AMPM would be provided
+    */
+    private static final BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> DEFAULT_ROUND_UP = (builder, parser) -> {
+        if (parser.toString().contains(ChronoField.DAY_OF_YEAR.toString())) {


Just a small comment here, since we might call this frequently enough and we don't know the cost of parser.toString(), do you mind extracting the parsed value into a local for the if conditionals?

…arch into rounding_date_parsing

ChrisHegarty

LGTM

pgomulka · 2022-09-01T08:19:27Z

I just realised that we possibly want to add a week based year defaulting too. So that we support YYYY'W'ww[e] ([ ] means parse optional day of week). It might be a bit artificial though, because I don't think [] syntax is commonly used (I might be mistaken..) and we don't have a named formatter with day of week within startOptional/endOptional

assuming [] is rare and thus the only optional fields that could conflict are month of year and day of month, this logic could be simplified to just

 if(parserAsString.contains(ChronoField.MONTH_OF_YEAR.toString()) ){
          builder.parseDefaulting(ChronoField.MONTH_OF_YEAR, 1L);
      }
      if(parserAsString.contains(ChronoField.DAY_OF_MONTH.toString()) ){
          builder.parseDefaulting(ChronoField.DAY_OF_MONTH, 1L);
      }

let me know what you think

pgomulka · 2022-09-01T08:44:24Z

@elasticmachine update branch

grcevski · 2022-09-01T13:42:55Z

I just realised that we possibly want to add a week based year defaulting too. So that we support YYYY'W'ww[e] ([ ] means parse optional day of week). It might be a bit artificial though, because I don't think [] syntax is commonly used (I might be mistaken..) and we don't have a named formatter with day of week within startOptional/endOptional

assuming [] is rare and thus the only optional fields that could conflict are month of year and day of month, this logic could be simplified to just
 if(parserAsString.contains(ChronoField.MONTH_OF_YEAR.toString()) ){
          builder.parseDefaulting(ChronoField.MONTH_OF_YEAR, 1L);
      }
      if(parserAsString.contains(ChronoField.DAY_OF_MONTH.toString()) ){
          builder.parseDefaulting(ChronoField.DAY_OF_MONTH, 1L);
      }
let me know what you think

I think that's a fair assumption. We don't have to make this more complicated at this point. I'd just put a comment that we don't handle the 'optional day of week' format, but adding day of month and month of year sound great!

grcevski · 2022-09-01T13:44:07Z

Since we are backporting to 7.17, can we please backport to 8.4.1 too, it's the current release.

…arch into rounding_date_parsing

Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes elastic#89096 closes elastic#58986

Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfMonth, MonthOfYear, ClockHourOfAMPM and HourOfAmPM closes #89096 closes #58986 backports #89693

Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes elastic#89096 closes elastic#58986

Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes #89096 closes #58986 backports #89693

in #89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes #90187

in elastic#89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes elastic#90187 (cherry picked from commit 3f3a95e)

in elastic#89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes elastic#90187 (cherry picked from commit 3f3a95e) # Conflicts: # server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java

in #89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes #90187 backports #90458

* Fix date rounding for date math parsing (#90458) in #89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes #90187 backport(#90458) (cherry picked from commit 3f3a95e)

…tic#90633) in elastic#89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes elastic#90187 backports elastic#90458

in #89693 the rounding logic was only applied when a field was present on a pattern. This is incorrect as for dates like "2020" we want to default to "2020-01-01T23:59:59.999..." when rounding is enabled. This commit always applies monthOfYear or dayofMonth defaulting (when rounding enabled) except when the dayOfYear is set closes #90187 backports #90458

pgomulka added 2 commits August 29, 2022 15:12

Merge remote-tracking branch 'upstream/main' into rounding_date_parsing

d6bc2aa

pgomulka added >bug :Core/Infra/Core Core issues without another label v8.5.0 v7.17.7 labels Aug 29, 2022

pgomulka self-assigned this Aug 29, 2022

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Aug 29, 2022

Update docs/changelog/89693.yaml

4fe069e

grcevski approved these changes Aug 29, 2022

View reviewed changes

pgomulka added 2 commits August 29, 2022 16:59

inline parser's tostring

2bd7f09

Merge branch 'rounding_date_parsing' of github.com:pgomulka/elasticse…

a0bfc53

…arch into rounding_date_parsing

ChrisHegarty approved these changes Aug 31, 2022

View reviewed changes

week based year support

57aa043

Merge branch 'main' into rounding_date_parsing

3a69b7c

pgomulka added the v8.4.1 label Sep 1, 2022

pgomulka added 2 commits September 5, 2022 09:57

simplify logic - do not allow for optional weekdays

4fe257d

Merge branch 'rounding_date_parsing' of github.com:pgomulka/elasticse…

79242a9

…arch into rounding_date_parsing

pgomulka merged commit 8e517da into elastic:main Sep 5, 2022

pgomulka mentioned this pull request Sep 5, 2022

[8.4] Extend the date rounding logic to be conditional (#89693) #89794

Merged

pgomulka mentioned this pull request Sep 5, 2022

Extend the date rounding logic to be conditional (#89693) #89801

Merged

romseygeek mentioned this pull request Sep 27, 2022

Match query on date property with format 'uuuu' (year) acts as a range query for years below 1971 #90187

Closed

pgomulka mentioned this pull request Sep 28, 2022

Fix date rounding for date math parsing #90458

Merged

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the date rounding logic to be conditional #89693

Extend the date rounding logic to be conditional #89693
pgomulka merged 9 commits intoelastic:mainfrom
pgomulka:rounding_date_parsing

pgomulka commented Aug 29, 2022

Uh oh!

elasticsearchmachine commented Aug 29, 2022

Uh oh!

elasticsearchmachine commented Aug 29, 2022

Uh oh!

grcevski left a comment

Uh oh!

grcevski Aug 29, 2022

Uh oh!

pgomulka Aug 29, 2022

Uh oh!

ChrisHegarty left a comment

Uh oh!

pgomulka commented Sep 1, 2022

Uh oh!

pgomulka commented Sep 1, 2022

Uh oh!

grcevski commented Sep 1, 2022

Uh oh!

grcevski commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

pgomulka commented Aug 29, 2022

Uh oh!

elasticsearchmachine commented Aug 29, 2022

Uh oh!

elasticsearchmachine commented Aug 29, 2022

Uh oh!

grcevski left a comment

Choose a reason for hiding this comment

Uh oh!

grcevski Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

pgomulka Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

ChrisHegarty left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Sep 1, 2022

Uh oh!

pgomulka commented Sep 1, 2022

Uh oh!

grcevski commented Sep 1, 2022

Uh oh!

grcevski commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants