Skip to content

Commit

Permalink
Fix java time epoch date formatters (#37829)
Browse files Browse the repository at this point in the history 
The self written epoch date formatters were not properly able to format
an Instant to a string due to a misconfiguration.

This fix also removes a until now existing runtime behaviour under java
8 regarding the names of the aggregation buckets, which are now the same
as before and have been under java 11.
  • Loading branch information
@spinscale
spinscale authored Feb 1, 2019
1 parent 859e2f5 commit c02cd3e
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 59 deletions.
51 changes: 14 additions & 37 deletions server/src/main/java/org/elasticsearch/common/time/EpochTime.java
View file
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);
}
@Override
public long getFrom(TemporalAccessor temporal) {
Expand Down Expand Up @@ -117,32 +115,30 @@ 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)
.optionalStart()
.appendFraction(NANOS_OF_MILLI, 0, 6, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

// this supports milliseconds ending in dot
Expand All @@ -151,32 +147,13 @@ public long getFrom(TemporalAccessor temporal) {
.appendLiteral('.')
.toFormatter(Locale.ROOT);

// this supports milliseconds with a fraction and is also used for printing
private static final DateTimeFormatter MILLISECONDS_FORMATTER3 = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER1)
.optionalStart() // optional is used so isSupported will be called when printing
.appendFraction(NANOS_OF_MILLI, 1, 6, true)
.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);

static final DateFormatter MILLIS_FORMATTER = getEpochMillisFormatter();
SECONDS_FORMATTER1, SECONDS_FORMATTER2);

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);

private abstract static class EpochField implements TemporalField {

Expand Down
20 changes: 20 additions & 0 deletions server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,26 @@ 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()));
assertThat(millisFormatter.format(Instant.ofEpochSecond(42, 0)), is("42000"));
assertThat(millisFormatter.format(Instant.ofEpochSecond(42, 123456789L)), is("42123.456789"));

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

assertThat(secondsFormatter.format(Instant.ofEpochSecond(42, 0)), is("42"));
}

public void testParsingStrictNanoDates() {
DateFormatter formatter = DateFormatters.forPattern("strict_date_optional_time_nanos");
formatter.format(formatter.parse("2016-01-01T00:00:00.000"));
Expand Down
28 changes: 6 additions & 22 deletions server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.SearchPhaseExecutionException;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.bootstrap.JavaVersion;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.script.Script;
Expand Down Expand Up @@ -996,39 +995,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

0 comments on commit c02cd3e

Please sign in to comment.