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

Fix java time epoch date formatters #37829

Merged
merged 12 commits into from
Feb 1, 2019
Merged

Fix java time epoch date formatters #37829

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
86593a9
Fix java time epoch date formatters
spinscale Jan 24, 2019
7934c18
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 25, 2019
caca187
added test
spinscale Jan 25, 2019
b7e4164
refactor by using a fraction with a minsize of 0
spinscale Jan 25, 2019
ff16e37
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 28, 2019
0757f67
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 28, 2019
6049d8e
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 28, 2019
124d5bd
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 29, 2019
da8cffb
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 29, 2019
812d667
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 30, 2019
6ac30ec
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 30, 2019
24c5179
Merge branch 'master' into 1901-fix-epoch-formatting
spinscale Jan 31, 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
40 changes: 11 additions & 29 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 @@ -19,8 +19,6 @@

package org.elasticsearch.common.time;

import org.elasticsearch.bootstrap.JavaVersion;

import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.time.format.ResolverStyle;
Expand Down Expand Up @@ -72,7 +70,7 @@ public TemporalAccessor resolve(Map<TemporalField,Long> fieldValues,
private static final EpochField NANOS_OF_SECOND = new EpochField(ChronoUnit.NANOS, ChronoUnit.SECONDS, ValueRange.of(0, 999_999_999)) {
@Override
public boolean isSupportedBy(TemporalAccessor temporal) {
return temporal.isSupported(ChronoField.NANO_OF_SECOND) && temporal.getLong(ChronoField.NANO_OF_SECOND) != 0;
return temporal.isSupported(ChronoField.NANO_OF_SECOND);
Copy link
Member

Choose a reason for hiding this comment

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

I added this condition to guard from printing out ".0" when nanos are 0. Why is this being removed? This test passes currently but fails with this PR:

DateFormatter secondsFormatter = DateFormatter.forPattern("epoch_second");
Instant instant = Instant.ofEpochSecond(42, 0);
assertThat(secondsFormatter.format(instant), equalTo("42"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, this one passed for me on java8 and java11 in this PR. Any test setup that I am missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the trick is, that you can have an optional fractional with a length of 0

}
@Override
public long getFrom(TemporalAccessor temporal) {
Expand Down Expand Up @@ -117,29 +115,24 @@ public boolean isSupportedBy(TemporalAccessor temporal) {
}
@Override
public long getFrom(TemporalAccessor temporal) {
return temporal.getLong(ChronoField.NANO_OF_SECOND);
return temporal.getLong(ChronoField.NANO_OF_SECOND) % 1_000_000;
}
};

// this supports seconds without any fraction
private static final DateTimeFormatter SECONDS_FORMATTER1 = new DateTimeFormatterBuilder()
.appendValue(SECONDS, 1, 19, SignStyle.NORMAL)
.optionalStart() // optional is used so isSupported will be called when printing
.appendFraction(NANOS_OF_SECOND, 0, 9, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

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

// this supports seconds with a fraction and is also used for printing
private static final DateTimeFormatter SECONDS_FORMATTER3 = new DateTimeFormatterBuilder()
.append(SECONDS_FORMATTER1)
.optionalStart() // optional is used so isSupported will be called when printing
.appendFraction(NANOS_OF_SECOND, 1, 9, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

// this supports milliseconds without any fraction
private static final DateTimeFormatter MILLISECONDS_FORMATTER1 = new DateTimeFormatterBuilder()
.appendValue(MILLIS, 1, 19, SignStyle.NORMAL)
Expand All @@ -159,24 +152,13 @@ public long getFrom(TemporalAccessor temporal) {
.optionalEnd()
.toFormatter(Locale.ROOT);

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER3,
static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER1,
builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L),
SECONDS_FORMATTER1, SECONDS_FORMATTER2, SECONDS_FORMATTER3);
SECONDS_FORMATTER1, SECONDS_FORMATTER2);

static final DateFormatter MILLIS_FORMATTER = getEpochMillisFormatter();

private static DateFormatter getEpochMillisFormatter() {
// the third formatter fails under java 8 as a printer, so fall back to this one
final DateTimeFormatter printer;
if (JavaVersion.current().getVersion().get(0) == 8) {
printer = MILLISECONDS_FORMATTER1;
} else {
printer = MILLISECONDS_FORMATTER3;
}
return new JavaDateFormatter("epoch_millis", printer,
builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L),
MILLISECONDS_FORMATTER1, MILLISECONDS_FORMATTER2, MILLISECONDS_FORMATTER3);
}
static final DateFormatter MILLIS_FORMATTER = new JavaDateFormatter("epoch_millis", MILLISECONDS_FORMATTER1,
builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L),
MILLISECONDS_FORMATTER1, MILLISECONDS_FORMATTER2, MILLISECONDS_FORMATTER3);

private abstract static class EpochField implements TemporalField {

Expand Down
16 changes: 16 additions & 0 deletions 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 @@ -147,6 +147,22 @@ public void testSupportBackwardsJava8Format() {
assertThat(formatter, instanceOf(JavaDateFormatter.class));
}

public void testEpochFormatting() {
long seconds = randomLongBetween(0, 130L * 365 * 86400); // from 1970 epoch till around 2100
long nanos = randomLongBetween(0, 999_999_999L);
Instant instant = Instant.ofEpochSecond(seconds, nanos);

DateFormatter millisFormatter = DateFormatter.forPattern("epoch_millis");
String millis = millisFormatter.format(instant);
Instant millisInstant = Instant.from(millisFormatter.parse(millis));
assertThat(millisInstant.toEpochMilli(), is(instant.toEpochMilli()));

DateFormatter secondsFormatter = DateFormatter.forPattern("epoch_second");
String formattedSeconds = secondsFormatter.format(instant);
Instant secondsInstant = Instant.from(secondsFormatter.parse(formattedSeconds));
assertThat(secondsInstant.getEpochSecond(), is(instant.getEpochSecond()));
}

public void testParsingStrictNanoDates() {
DateFormatter formatter = DateFormatters.forPattern("strict_date_optional_time_nanos");
formatter.format(formatter.parse("2016-01-01T00:00:00.000"));
Expand Down
27 changes: 6 additions & 21 deletions server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,39 +996,24 @@ public void testRangeWithFormatNumericValue() throws Exception {
.addAggregation(dateRange("date_range").field("date").addRange(1000, 3000).addRange(3000, 4000)).get();
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
List<Bucket> buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2);
if (JavaVersion.current().getVersion().get(0) == 8) {
assertBucket(buckets.get(0), 2L, "1000.0-3000.0", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000.0-4000.0", 3000000L, 4000000L);
} else {
assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L);
}
assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L);

// using no format should also work when and to/from are string values
searchResponse = client().prepareSearch(indexName).setSize(0)
.addAggregation(dateRange("date_range").field("date").addRange("1000", "3000").addRange("3000", "4000")).get();
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2);
if (JavaVersion.current().getVersion().get(0) == 8) {
assertBucket(buckets.get(0), 2L, "1000.0-3000.0", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000.0-4000.0", 3000000L, 4000000L);
} else {
assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L);
}
assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L);

// also e-notation should work, fractional parts should be truncated
searchResponse = client().prepareSearch(indexName).setSize(0)
.addAggregation(dateRange("date_range").field("date").addRange(1.0e3, 3000.8123).addRange(3000.8123, 4.0e3)).get();
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2);
if (JavaVersion.current().getVersion().get(0) == 8) {
assertBucket(buckets.get(0), 2L, "1000.0-3000.0", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000.0-4000.0", 3000000L, 4000000L);
} else {
assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L);
}
assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L);
assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L);

// using different format should work when to/from is compatible with
// format in aggregation
Expand Down