Skip to content

Commit

Permalink
Set start of the week to Monday for root locale (elastic#43652)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
pgomulka authored Aug 9, 2019
1 parent af70fb9 commit 8d1ea86
Show file tree
Hide file tree
Showing 10 changed files with 343 additions and 207 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.common.time;

import java.util.Locale;

/**
* Locale constants to be used across elasticsearch code base.
* java.util.Locale.ROOT should not be used as it defaults start of the week incorrectly to Sunday.
*/
public final class IsoLocale {
private IsoLocale() {
throw new UnsupportedOperationException();
}

/**
* We want to use Locale.ROOT but with a start of the week as defined in ISO8601 to be compatible with the behaviour in joda-time
* https://github.com/elastic/elasticsearch/issues/42588
* @see java.time.temporal.WeekFields#of(Locale)
*/
public static final Locale ROOT = new Locale.Builder()
.setLocale(Locale.ROOT)
.setUnicodeLocaleKeyword("fw", "mon").build();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
---
setup:
- skip:
version: " - 7.99.99" #TODO change this after backport
reason: "not backported yet"

- do:
indices.create:
index: test
body:
mappings:
properties:
date:
type: date

- do:
index:
index: test
id: 1
body: { "date": "2009-11-15T14:12:12" }

- do:
indices.refresh:
index: [test]

---
# The inserted document has a field date=2009-11-15T14:12:12 which is Sunday.
# When aggregating per day of the week this should be considered as last day of the week (7)
# and this value should be used in 'key_as_string'
"Date aggregartion per day of week":
- do:
search:
rest_total_hits_as_int: true
index: test
body:
aggregations:
test:
"date_histogram": {
"field": "date",
"calendar_interval": "day",
"format": "e",
"offset": 0
}

- match: {hits.total: 1}
- length: { aggregations.test.buckets: 1 }
- match: { aggregations.test.buckets.0.key_as_string: "7" }
409 changes: 216 additions & 193 deletions server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -125,27 +125,27 @@ public long getFrom(TemporalAccessor temporal) {
.optionalStart() // optional is used so isSupported will be called when printing
.appendFraction(NANOS_OF_SECOND, 0, 9, true)
.optionalEnd()
.toFormatter(Locale.ROOT);
.toFormatter(IsoLocale.ROOT);

// this supports seconds ending in dot
private static final DateTimeFormatter SECONDS_FORMATTER2 = new DateTimeFormatterBuilder()
.appendValue(SECONDS, 1, 19, SignStyle.NORMAL)
.appendLiteral('.')
.toFormatter(Locale.ROOT);
.toFormatter(IsoLocale.ROOT);

// this supports milliseconds without any fraction
private static final DateTimeFormatter MILLISECONDS_FORMATTER1 = new DateTimeFormatterBuilder()
.appendValue(MILLIS, 1, 19, SignStyle.NORMAL)
.optionalStart()
.appendFraction(NANOS_OF_MILLI, 0, 6, true)
.optionalEnd()
.toFormatter(Locale.ROOT);
.toFormatter(IsoLocale.ROOT);

// this supports milliseconds ending in dot
private static final DateTimeFormatter MILLISECONDS_FORMATTER2 = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER1)
.appendLiteral('.')
.toFormatter(Locale.ROOT);
.toFormatter(IsoLocale.ROOT);

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER1,
builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.elasticsearch.common.time.DateFormatters;
import org.elasticsearch.common.time.DateMathParser;
import org.elasticsearch.common.time.DateUtils;
import org.elasticsearch.common.time.IsoLocale;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
Expand Down Expand Up @@ -135,7 +136,7 @@ public static class Builder extends FieldMapper.Builder<Builder, DateFieldMapper
public Builder(String name) {
super(name, new DateFieldType(), new DateFieldType());
builder = this;
locale = Locale.ROOT;
locale = IsoLocale.ROOT;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.elasticsearch.common.collect.CopyOnWriteHashMap;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.time.IsoLocale;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
Expand All @@ -41,7 +42,6 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Map;

public class ObjectMapper extends Mapper implements Cloneable {
Expand Down Expand Up @@ -530,7 +530,7 @@ public void toXContent(XContentBuilder builder, Params params, ToXContent custom
builder.field("type", CONTENT_TYPE);
}
if (dynamic != null) {
builder.field("dynamic", dynamic.name().toLowerCase(Locale.ROOT));
builder.field("dynamic", dynamic.name().toLowerCase(IsoLocale.ROOT));
}
if (enabled != Defaults.ENABLED) {
builder.field("enabled", enabled);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.time.DateMathParser;
import org.elasticsearch.common.time.IsoLocale;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand Down Expand Up @@ -94,7 +95,7 @@ public static class Defaults {

public static class Builder extends FieldMapper.Builder<Builder, RangeFieldMapper> {
private Boolean coerce;
private Locale locale = Locale.ROOT;
private Locale locale = IsoLocale.ROOT;
private String pattern;

public Builder(String name, RangeType type) {
Expand Down Expand Up @@ -413,7 +414,7 @@ && fieldType().dateTimeFormatter().pattern().equals(DateFieldMapper.DEFAULT_DATE
}
if (fieldType().rangeType == RangeType.DATE
&& (includeDefaults || (fieldType().dateTimeFormatter() != null
&& fieldType().dateTimeFormatter().locale() != Locale.ROOT))) {
&& fieldType().dateTimeFormatter().locale() != IsoLocale.ROOT))) {
builder.field("locale", fieldType().dateTimeFormatter().locale());
}
if (includeDefaults || coerce.explicit()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.joda.time.DateTimeZone;
import org.joda.time.format.ISODateTimeFormat;

import java.time.LocalDateTime;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
Expand All @@ -35,8 +36,31 @@

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.core.IsEqual.equalTo;

public class JavaJodaTimeDuellingTests extends ESTestCase {
@Override
protected boolean enableWarningsCheck() {
return false;
}

public void testDayOfWeek() {
//7 (ok joda) vs 1 (java by default) but 7 with customized org.elasticsearch.common.time.IsoLocale.ISO8601
ZonedDateTime now = LocalDateTime.of(2009,11,15,1,32,8,328402)
.atZone(ZoneOffset.UTC); //Sunday
DateFormatter jodaFormatter = Joda.forPattern("e").withLocale(Locale.ROOT).withZone(ZoneOffset.UTC);
DateFormatter javaFormatter = DateFormatter.forPattern("8e").withZone(ZoneOffset.UTC);
assertThat(jodaFormatter.format(now), equalTo(javaFormatter.format(now)));
}

public void testStartOfWeek() {
//2019-21 (ok joda) vs 2019-22 (java by default) but 2019-21 with customized org.elasticsearch.common.time.IsoLocale.ISO8601
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").withZone(ZoneOffset.UTC);
assertThat(jodaFormatter.format(now), equalTo(javaFormatter.format(now)));
}

//these parsers should allow both ',' and '.' as a decimal point
public void testDecimalPointParsing(){
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public void testParsersWithMultipleInternalFormats() throws Exception {
}

public void testLocales() {
assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(Locale.ROOT));
assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(IsoLocale.ROOT));
Locale locale = randomLocale(random());
assertThat(DateFormatters.forPattern("strict_date_optional_time").withLocale(locale).locale(), is(locale));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@

package org.elasticsearch.xpack.sql.proto;

import org.elasticsearch.common.time.IsoLocale;

import java.sql.Timestamp;
import java.time.Duration;
import java.time.OffsetTime;
import java.time.Period;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.util.Locale;
import java.util.Objects;
import java.util.concurrent.TimeUnit;

Expand All @@ -24,7 +25,6 @@
import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;

public final class StringUtils {

public static final String EMPTY = "";

public static final DateTimeFormatter ISO_DATE_WITH_MILLIS = new DateTimeFormatterBuilder()
Expand All @@ -38,7 +38,7 @@ public final class StringUtils {
.appendValue(SECOND_OF_MINUTE, 2)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendOffsetId()
.toFormatter(Locale.ROOT);
.toFormatter(IsoLocale.ROOT);

public static final DateTimeFormatter ISO_TIME_WITH_MILLIS = new DateTimeFormatterBuilder()
.parseCaseInsensitive()
Expand All @@ -49,7 +49,7 @@ public final class StringUtils {
.appendValue(SECOND_OF_MINUTE, 2)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendOffsetId()
.toFormatter(Locale.ROOT);
.toFormatter(IsoLocale.ROOT);

private static final int SECONDS_PER_MINUTE = 60;
private static final int SECONDS_PER_HOUR = SECONDS_PER_MINUTE * 60;
Expand Down

0 comments on commit 8d1ea86

Please sign in to comment.