[Java.time] Calculate week of a year with ISO rules #48209

pgomulka · 2019-10-17T18:44:44Z

IsoLocale - a class introduced to configure start of the week as Monday (with the use of extension) does not allow to configure minimum number of days in a first week. This does not allow correctly calculate week of a week-based-year with ISO rules.
ISO rule is: First day of a week is Monday AND minimum number of days in a first week of a year is 4.

This change is to refactor this with the use of CalendarDataProvider and requires a jvm property java.locale.providers to be set with SPI,COMPAT. SPI is for this change, COMPAT was already used to be compatible with JDK8 date behaviour.
Previously not all tests were using COMPAT, so had to be changed now (notice the change in how dates are represented without . in text forms)

SQL plugin is at the moment configured to use SUNDAY rule - meaning First of the week is Sunday and requires only 1 day in a week to be the first week of the year. This was discussed with SQL team, to keep non-iso functions rely on Sunday,1 rule. cc @astefan
A link to consider https://docs.microsoft.com/en-us/sql/t-sql/functions/datepart-transact-sql?view=sql-server-ver15#week-and-weekday-datepart-arguments

closes #41670 a bug raised where week is wrongly calculated
relates #43652 the previous attempt to just change start of the week

relates #42588 an issue with investigation details
relates #43275 a bug related to week starting on SUNDAY (fixed by #43652 but also by this PR)

This reverts commit 8d1ea86.

Introducing a IsoLocal.ROOT constant which should be used instead of java.util.Locale.ROOT in ES when dealing with dates. IsoLocal.ROOT customises start of the week to be Monday instead of Sunday. closes elastic#42588 an issue with investigation details relates elastic#41670 bug raised (this won't fix it on its own. joda.parseInto has to be reimplemented closes elastic#43275 an issue raised by community member change skip reason compile error not orking spi working unit test cleanup change providers for 9+ revert changes IsoLocale cleanup move spi files to server make unit test pass from gradle expermienting with gradle tasks uncomment jar hell check only add settings in buildplugin allign options for locale providers

elasticmachine · 2019-10-21T13:05:04Z

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

pgomulka · 2019-10-21T13:06:27Z

I think this should be merged to v8 and v7.6. We still support jdk8 in ESv7 and this approach won't work (unless CalendarDataProvider is shipped in a separate jar into jre ext directory), but it will work for jdk9+

pgomulka · 2019-10-21T17:36:01Z

During the SQL team meeting we decided to keep the current implementation of date functions as they are. However I think they need to be fixed too as I found some problems there.
I think it is worth visiting pgomulka#8 (against this branch) before finishing with this current PR.

rjernst

LGTM

rjernst · 2019-10-21T17:47:41Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -857,6 +857,8 @@ class BuildPlugin implements Plugin<Project> {
                        'tests.security.manager': 'true',
                        'jna.nosys': 'true'

+                test.systemProperty ('java.locale.providers','SPI,COMPAT')


Could you please add a comment that this won't be needed once we include jvm.options in tests sysprops?

In #48209 PR unused import and redundant else statements were missed. This commit cleans this up

tvernum · 2019-11-21T04:15:55Z

x-pack/plugin/core/src/test/java/org/elasticsearch/license/LicenseServiceTests.java

-    }
-
-    public void testLogExpirationWarningOnJdk8() {
+    public void testLogExpirationWarning() {
        assumeTrue("this is for JDK8 only", JavaVersion.current().equals(JavaVersion.parse("8")));


@pgomulka I think this part of the change might have been incorrect...
We collapsed the 2 tests into 1, but that one test is set to only run on JDK8. I think this assumeTrue should be removed?

@tvernum you are right, the same applies to 7.x branch as the output should be the same.
thanks for taking a look!

Some extended testing with MS-SQL server and H2 (which agree on results) revealed bugs in the implementation of WEEK related extraction and diff functions. Non-iso WEEK seems to be broken since elastic#48209 because of the replacement of Calendar and the change in the ISO rules. ISO_WEEK failed for some edge cases around the January 1st. DATE_DIFF was previously based on non-iso WEEK extraction which seems not to be the case. Fixes: elastic#49376

This test no longer relies on jdk version, so the assume should be removed relates #48209

This test no longer relies on jdk version, so the assume should be removed relates elastic#48209

This test no longer relies on jdk version, so the assume should be removed relates #48209

Some extended testing with MS-SQL server and H2 (which agree on results) revealed bugs in the implementation of WEEK related extraction and diff functions. Non-iso WEEK seems to be broken since #48209 because of the replacement of Calendar and the change in the ISO rules. ISO_WEEK failed for some edge cases around the January 1st. DATE_DIFF was previously based on non-iso WEEK extraction which seems not to be the case. Fixes: #49376

Some extended testing with MS-SQL server and H2 (which agree on results) revealed bugs in the implementation of WEEK related extraction and diff functions. Non-iso WEEK seems to be broken since #48209 because of the replacement of Calendar and the change in the ISO rules. ISO_WEEK failed for some edge cases around the January 1st. DATE_DIFF was previously based on non-iso WEEK extraction which seems not to be the case. Fixes: #49376 (cherry picked from commit 54fe7f5)

The test should be run against 7.7 version at least, as this was only backported and released in that version relates SPI based implementation #48209 relates backport #50916

Some extended testing with MS-SQL server and H2 (which agree on results) revealed bugs in the implementation of WEEK related extraction and diff functions. Non-iso WEEK seems to be broken since elastic#48209 because of the replacement of Calendar and the change in the ISO rules. ISO_WEEK failed for some edge cases around the January 1st. DATE_DIFF was previously based on non-iso WEEK extraction which seems not to be the case. Fixes: elastic#49376

pgomulka added 3 commits October 17, 2019 16:53

Revert "Set start of the week to Monday for root locale (elastic#43652)"

8c90164

This reverts commit 8d1ea86.

working parsing of weekbased fields

dc0c953

pgomulka added the WIP label Oct 17, 2019

pgomulka added 10 commits October 17, 2019 21:15

checkstyle

352344b

support for weekbased dates convertions to zoned

8ca12f0

do not use ISOWeekFields

2eb27f5

adding tests and gradle setting

88dc041

sql test fix

ac5b55b

test fix

b7ae80c

fix license service test

207a8ea

fix test when COMPAT is used

963e3f1

Merge branch 'master' into feature/joda-min-days-spi

acc28fc

test fix for master only

ac60ba3

pgomulka changed the title ~~WIP Feature/joda min days spi~~ [Java.time] Correctly calculate week of a year Oct 21, 2019

pgomulka changed the title ~~[Java.time] Correctly calculate week of a year~~ [Java.time] Calculate week of a year with ISO rules Oct 21, 2019

pgomulka marked this pull request as ready for review October 21, 2019 13:04

pgomulka requested review from astefan and rjernst October 21, 2019 13:04

pgomulka self-assigned this Oct 21, 2019

pgomulka added the :Core/Infra/Core Core issues without another label label Oct 21, 2019

pgomulka added v7.6.0 v8.0.0 and removed WIP labels Oct 21, 2019

rjernst approved these changes Oct 21, 2019

View reviewed changes

comment to a unit test

45fe6f7

astefan requested a review from matriv October 22, 2019 08:02

pgomulka added a commit that referenced this pull request Oct 24, 2019

Remove redundant import and else statement (#48352)

e9ffbd1

In #48209 PR unused import and redundant else statements were missed. This commit cleans this up

tvernum reviewed Nov 21, 2019

View reviewed changes

pgomulka mentioned this pull request Nov 21, 2019

Enable LicenceServiceTests for all jdks #49440

Merged

matriv mentioned this pull request Nov 27, 2019

Fix issues with WEEK/ISO_WEEK/DATEDIFF #49405

Merged

pgomulka added a commit that referenced this pull request Nov 28, 2019

Enable LicenceServiceTests for all jdks (#49440)

7a1720c

This test no longer relies on jdk version, so the assume should be removed relates #48209

pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Nov 28, 2019

Enable LicenceServiceTests for all jdks (elastic#49440)

f8032ec

This test no longer relies on jdk version, so the assume should be removed relates elastic#48209

pgomulka mentioned this pull request Nov 28, 2019

Enable LicenceServiceTests for all jdks (#49440) #49682

Merged

pgomulka added a commit that referenced this pull request Nov 28, 2019

Enable LicenceServiceTests for all jdks (#49440) backport(#49682)

e528b41

This test no longer relies on jdk version, so the assume should be removed relates #48209

pgomulka mentioned this pull request Jan 13, 2020

Remove TODO in a test for verifying start of the week #50894

Merged

$@polyfractal$ polyfractal added the >bug label Jan 15, 2020

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

pgomulka mentioned this pull request Jun 25, 2020

Index name expression resolver bwc layer for date parsing #58503

Merged

ywangd mentioned this pull request Jul 30, 2020

SimpleKdcLdapServerTests testClientServiceMutualAuthentication fails on JDK15-ea #57749

Closed

pgomulka mentioned this pull request Oct 13, 2020

Ingest pipeline parses the wrong week of the year #63489

Closed

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

ralgozino mentioned this pull request Dec 16, 2021

[DO NOT MERGE THIS PR] Prepare for release v1.5.1 sighupio/fury-kubernetes-logging#57

Closed

This was referenced Sep 12, 2024

Change default locale of date processors to ENGLISH #112796

Merged

Use ISO week date numbering for english too #112801

Closed

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java.time] Calculate week of a year with ISO rules #48209

[Java.time] Calculate week of a year with ISO rules #48209

pgomulka commented Oct 17, 2019 •

edited

Loading

elasticmachine commented Oct 21, 2019

pgomulka commented Oct 21, 2019

pgomulka commented Oct 21, 2019

rjernst left a comment

rjernst Oct 21, 2019

tvernum Nov 21, 2019

pgomulka Nov 21, 2019

[Java.time] Calculate week of a year with ISO rules #48209

[Java.time] Calculate week of a year with ISO rules #48209

Conversation

pgomulka commented Oct 17, 2019 • edited Loading

elasticmachine commented Oct 21, 2019

pgomulka commented Oct 21, 2019

pgomulka commented Oct 21, 2019

rjernst left a comment

Choose a reason for hiding this comment

rjernst Oct 21, 2019

Choose a reason for hiding this comment

tvernum Nov 21, 2019

Choose a reason for hiding this comment

pgomulka Nov 21, 2019

Choose a reason for hiding this comment

pgomulka commented Oct 17, 2019 •

edited

Loading