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

Set start of the week to Monday for root locale #43652

Merged
merged 15 commits into from
Aug 9, 2019
34 changes: 34 additions & 0 deletions libs/core/src/main/java/org/elasticsearch/common/time/Locale.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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;

/**
* 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 class Locale {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a different class name that does not collide with java's Locale, so that we don't need to deal with fully qualified class names? Perhaps IsoLocale (we should also make this class not constructable).

/**
* 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(java.util.Locale)
*/
public static final java.util.Locale ISO8601 = new java.util.Locale.Builder()
.setLocale(java.util.Locale.ROOT)
.setUnicodeLocaleKeyword("fw", "mon").build();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
---
setup:
- 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 @@ -30,7 +30,6 @@
import java.time.temporal.TemporalField;
import java.time.temporal.TemporalUnit;
import java.time.temporal.ValueRange;
import java.util.Locale;
import java.util.Map;

/**
Expand Down Expand Up @@ -125,27 +124,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(Locale.ISO8601);

// 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(Locale.ISO8601);

// 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(Locale.ISO8601);

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

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER1,
builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L),
Expand All @@ -168,7 +167,7 @@ private EpochField(TemporalUnit baseUnit, TemporalUnit rangeUnit, ValueRange ran
}

@Override
public String getDisplayName(Locale locale) {
public String getDisplayName(java.util.Locale locale) {
return toString();
}

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.Locale;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
Expand All @@ -59,7 +60,6 @@
import java.time.ZoneOffset;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;

Expand Down Expand Up @@ -129,13 +129,13 @@ public static class Builder extends FieldMapper.Builder<Builder, DateFieldMapper

private Boolean ignoreMalformed;
private Explicit<String> format = new Explicit<>(DEFAULT_DATE_TIME_FORMATTER.pattern(), false);
private Locale locale;
private java.util.Locale locale;
private Resolution resolution = Resolution.MILLISECONDS;

public Builder(String name) {
super(name, new DateFieldType(), new DateFieldType());
builder = this;
locale = Locale.ROOT;
locale = Locale.ISO8601;
}

@Override
Expand All @@ -158,12 +158,12 @@ protected Explicit<Boolean> ignoreMalformed(BuilderContext context) {
return Defaults.IGNORE_MALFORMED;
}

public Builder locale(Locale locale) {
public Builder locale(java.util.Locale locale) {
this.locale = locale;
return this;
}

public Locale locale() {
public java.util.Locale locale() {
return locale;
}

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.Locale;
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(Locale.ISO8601));
}
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.Locale;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand All @@ -68,7 +69,6 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand All @@ -94,7 +94,7 @@ public static class Defaults {

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

public Builder(String name, RangeType type) {
Expand Down Expand Up @@ -140,7 +140,7 @@ public Builder nullValue(Object nullValue) {
throw new IllegalArgumentException("Field [" + name() + "] does not support null value.");
}

public void locale(Locale locale) {
public void locale(java.util.Locale locale) {
this.locale = locale;
}

Expand Down Expand Up @@ -413,7 +413,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() != Locale.ISO8601))) {
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.Locale.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.Locale.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 @@ -29,7 +29,6 @@
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoField;
import java.time.temporal.TemporalAccessor;
import java.util.Locale;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -116,8 +115,8 @@ public void testParsersWithMultipleInternalFormats() throws Exception {
}

public void testLocales() {
assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(Locale.ROOT));
Locale locale = randomLocale(random());
assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(Locale.ISO8601));
java.util.Locale locale = randomLocale(random());
assertThat(DateFormatters.forPattern("strict_date_optional_time").withLocale(locale).locale(), is(locale));
}

Expand All @@ -139,8 +138,8 @@ public void testEqualsAndHashcode() {
assertThat(DateFormatters.forPattern("YYYY").withZone(ZoneId.of("CET")), not(equalTo(DateFormatters.forPattern("YYYY"))));

// different locale, thus not equals
DateFormatter f1 = DateFormatters.forPattern("YYYY").withLocale(Locale.CANADA);
DateFormatter f2 = f1.withLocale(Locale.FRENCH);
DateFormatter f1 = DateFormatters.forPattern("YYYY").withLocale(java.util.Locale.CANADA);
DateFormatter f2 = f1.withLocale(java.util.Locale.FRENCH);
assertThat(f1, not(equalTo(f2)));

// different pattern, thus not equals
Expand Down Expand Up @@ -296,7 +295,7 @@ public void testRoundupFormatterZone() {
}

public void testRoundupFormatterLocale() {
Locale locale = randomLocale(random());
java.util.Locale locale = randomLocale(random());
String format = randomFrom("epoch_second", "epoch_millis", "strict_date_optional_time", "uuuu-MM-dd'T'HH:mm:ss.SSS",
"strict_date_optional_time||date_optional_time");
JavaDateFormatter formatter = (JavaDateFormatter) DateFormatter.forPattern(format).withLocale(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.Locale;

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(Locale.ISO8601);

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(Locale.ISO8601);

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