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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
Aug 9, 2019
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9ab7c92
set start of the week to monday for root locale
pgomulka Jun 26, 2019
646bc7d
make more code use changed root
pgomulka Jun 27, 2019
ead2668
dummy fix for checkstyle
pgomulka Jun 27, 2019
029c015
fix checkstyle
pgomulka Jun 27, 2019
5249b4c
fix root
pgomulka Jun 27, 2019
cea5410
Merge branch 'master' into fix/start_of_week
pgomulka Jun 27, 2019
e5084f5
test fix
pgomulka Jun 27, 2019
f8a9279
class rename and move to libs/cli/core
pgomulka Aug 5, 2019
95ab0c0
Merge branch 'master' into fix/start_of_week
pgomulka Aug 6, 2019
35dd579
yml test
pgomulka Aug 6, 2019
47b3e5b
typo
pgomulka Aug 6, 2019
c99048e
unit test
pgomulka Aug 6, 2019
9a80fc2
rename the locale class
pgomulka Aug 8, 2019
bd05fe5
Merge branch 'master' into fix/start_of_week
pgomulka Aug 9, 2019
e1218ea
skip the yml teste until backported
pgomulka Aug 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
403 changes: 211 additions & 192 deletions server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions server/src/main/java/org/elasticsearch/common/time/EpochTime.java
View file
Open in desktop
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(RLocale.INS);

// 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(RLocale.INS);

// 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(RLocale.INS);

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

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER1,
builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L),
Expand Down
25 changes: 25 additions & 0 deletions server/src/main/java/org/elasticsearch/common/time/RLocale.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* 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;

public class RLocale {
public static final java.util.Locale INS = new java.util.Locale.Builder()
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 put this in DateUtils instead of creating a new class? I also don't know what "INS" is supposed to mean. Maybe ISO8601_LOCALE?

Copy link
Contributor Author

@pgomulka pgomulka Jul 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

RLocale.INS was same length as Locale.ROOT - just wanted to quickly see if this passes CI and didn't want to fix all line length violations.
The name obviously ha to be changed - ISO8601_LOCALE sounds good to me.

.setLocale(java.util.Locale.ROOT)
.setUnicodeLocaleKeyword("fw", "mon").build();
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment explaining why this is set to monday, and linking to the appropriate java discussion/code/iso definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

}
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
View file
Open in desktop
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.RLocale;
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 = RLocale.INS;
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
View file
Open in desktop
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.RLocale;
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(RLocale.INS));
}
if (enabled != Defaults.ENABLED) {
builder.field("enabled", enabled);
Expand Down
5 changes: 3 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java
View file
Open in desktop
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.RLocale;
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 = RLocale.INS;
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() != RLocale.INS))) {
builder.field("locale", fieldType().dateTimeFormatter().locale());
}
if (includeDefaults || coerce.explicit()) {
Expand Down
15 changes: 15 additions & 0 deletions server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java
View file
Open in desktop
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,22 @@

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 testStartOfWeek() {
//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").withZone(ZoneOffset.UTC);
assertThat(jodaFormatter.format(now), equalTo(javaFormatter.format(now)));
}

public void testTimeZoneFormatting() {
assertSameDate("2001-01-01T00:00:00Z", "date_time_no_millis");
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java
View file
Open in desktop
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(RLocale.INS));
Locale locale = randomLocale(random());
assertThat(DateFormatters.forPattern("strict_date_optional_time").withLocale(locale).locale(), is(locale));
}
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
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,6 +23,8 @@
import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;

public final class StringUtils {
public static final java.util.Locale WEEK_START_MON = new java.util.Locale.Builder()
Copy link
Member

Choose a reason for hiding this comment

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

Why another copy? Can we use a single shared locale with this set to monday?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was here because the sql project is not depending on server

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then maybe do put it into it's own class, but in core/libs? It is all jdk based stuff so it should be fine there.

.setLocale(java.util.Locale.ROOT).setUnicodeLocaleKeyword("fw", "mon").build();

public static final String EMPTY = "";

Expand All @@ -38,7 +39,7 @@ public final class StringUtils {
.appendValue(SECOND_OF_MINUTE, 2)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendOffsetId()
.toFormatter(Locale.ROOT);
.toFormatter(WEEK_START_MON);

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

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