Skip to content
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

Use ISO standard for week of week based year #42588

Closed
pgomulka opened this issue May 27, 2019 · 3 comments
Closed

Use ISO standard for week of week based year #42588

pgomulka opened this issue May 27, 2019 · 3 comments
Assignees
Labels
>bug :Core/Infra/Core Core issues without another label

Comments

@pgomulka
Copy link
Contributor

In java.time week of weekbased year is depending on Locale. The locale we use is ROOT, and it does not define the start of the week.
Therefore start of the week can be either Sunday or Monday.
ISO 8601 defines start of the week as Monday, and that standard was used in Joda for week of week based year.
This would fail because of that change.
2019-21 (ok joda) vs 2019-22 (java)

 ZonedDateTime now = LocalDateTime.of(2019,5,26,1,32,8,328402)
                                         .atZone(ZoneOffset.UTC);
        DateFormatter jodaFormatter = Joda.forPattern("xxxx-ww").withLocale(Locale.ROOT).withZone(ZoneOffset.UTC);
        DateFormatter javaFormatter = DateFormatter.forPattern("8YYYY-ww").withLocale(Locale.ROOT).withZone(ZoneOffset.UTC);
        assertThat(jodaFormatter.format(now), equalTo(javaFormatter.format(now)));
@pgomulka pgomulka added >bug :Core/Infra/Core Core issues without another label labels May 27, 2019
@pgomulka pgomulka self-assigned this May 27, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 3, 2019

This behaviour is described in jdk javadoc

    w       1      append special localized WeekFields element for numeric week-of-year

It mentioned that this is localized. The code that is responisble for this is here:

1 = {StackTraceElement@4560} "java.base/java.time.temporal.WeekFields.of(WeekFields.java:299)"
2 = {StackTraceElement@4561} "java.base/java.time.format.DateTimeFormatterBuilder$WeekBasedFieldPrinterParser.printerParser(DateTimeFormatterBuilder.java:4882)"
3 = {StackTraceElement@4562} "java.base/java.time.format.DateTimeFormatterBuilder$WeekBasedFieldPrinterParser.format(DateTimeFormatterBuilder.java:4866)"
4 = {StackTraceElement@4563} "java.base/java.time.format.DateTimeFormatterBuilder$CompositePrinterParser.format(DateTimeFormatterBuilder.java:2345)"
5 = {StackTraceElement@4564} "java.base/java.time.format.DateTimeFormatter.formatTo(DateTimeFormatter.java:1846)"
6 = {StackTraceElement@4565} "java.base/java.time.format.DateTimeFormatter.format(DateTimeFormatter.java:1820)"
7 = {StackTraceElement@4566} 

pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Aug 9, 2019
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
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Oct 17, 2019
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
@pgomulka
Copy link
Contributor Author

raising this for the record. Will update known issues
related to this bug #41670 and fixed in v7.6+ PR fix #48349
affecting 6.8 - 7.5

week based dates like 2020-01 were not correctly translated to a long epoch value and throwing an exception. Week based values should be defaulted to start of the week.
reproduce

curl --request PUT \
  --url http://localhost:9200/test \
  --header 'authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==' \
  --header 'content-type: application/json' \
  --data '{
	"mappings" :{

			"properties": {
				"timestamp": {
					"type": "date",
						"format":"YYYY-ww",
					"store": true
					}
			}
		}

}'
curl --request POST \
  --url http://localhost:9200/test/_doc \
  --header 'authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==' \
  --header 'content-type: application/json' \
  --data '{
	"timestamp": "2020-33"
	
}'

results in

{
  "error": {
    "root_cause": [
      {
        "type": "mapper_parsing_exception",
        "reason": "failed to parse field [timestamp] of type [date] in document with id 'Q9-p5XIBy0lAoBkyOJdN'"
      }
    ],
    "type": "mapper_parsing_exception",
    "reason": "failed to parse field [timestamp] of type [date] in document with id 'Q9-p5XIBy0lAoBkyOJdN'",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "temporal accessor [{WeekBasedYear[WeekFields[SUNDAY,1]]=2020, WeekOfWeekBasedYear[WeekFields[SUNDAY,1]]=33},ISO] cannot be converted to zoned date time"
    }
  },
  "status": 400
}

should not, and since 7.6 it returns fine.
then when searching

curl --request GET \
  --url http://localhost:9200/test/_search \
  --header 'authorization: Basic ZWxhc3RpYzpwYXNzd29yZA==' \
  --header 'content-type: application/json' \
  --data '{
	  "stored_fields": [ "timestamp" ] 

}'

correct

{
  "took": 4,
  "timed_out": false,
  "_shards": {
    "total": 1,
    "successful": 1,
    "skipped": 0,
    "failed": 0
  },
  "hits": {
    "total": {
      "value": 1,
      "relation": "eq"
    },
    "max_score": 1.0,
    "hits": [
      {
        "_index": "test",
        "_type": "_doc",
        "_id": "1Pqe5XIB1L6riJIjbO85",
        "_score": 1.0,
        "fields": {
          "timestamp": [
            "2020-33"
          ]
        }
      }
    ]
  }
}

@pgomulka pgomulka reopened this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label
Projects
None yet
Development

No branches or pull requests

2 participants