Skip to content

Commit

Permalink
fixed an issue where epoch millis were not being rounded up but inste…
Browse files Browse the repository at this point in the history 
…ad were rounded down causing gt behavior to fail when off by 1 millisecond
  • Loading branch information
@john-wagster
john-wagster committed Dec 10, 2024
1 parent 39c7e0b commit 190bb7c
Show file tree
Hide file tree
Showing 3 changed files with 337 additions and 26 deletions.
109 changes: 86 additions & 23 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 @@ -35,6 +35,25 @@ class EpochTime {

private static final ValueRange POSITIVE_LONG_INTEGER_RANGE = ValueRange.of(0, Long.MAX_VALUE);

// TemporalField is only present in the presence of a rounded timestamp
private static final long ROUNDED_SIGN_PLACEHOLDER = -2;
private static final EpochField ROUNDED_SIGN_FIELD = new EpochField(
ChronoUnit.FOREVER,
ChronoUnit.FOREVER,
ValueRange.of(ROUNDED_SIGN_PLACEHOLDER, ROUNDED_SIGN_PLACEHOLDER)
) {
// FIXME: what should this be?
@Override
public boolean isSupportedBy(TemporalAccessor temporal) {
return temporal.isSupported(ChronoField.INSTANT_SECONDS) && temporal.getLong(ChronoField.INSTANT_SECONDS) < 0;
}

@Override
public long getFrom(TemporalAccessor temporal) {
return ROUNDED_SIGN_PLACEHOLDER;
}
};

// TemporalField is only present in the presence of a negative (potentially fractional) timestamp.
private static final long NEGATIVE_SIGN_PLACEHOLDER = -1;
private static final EpochField NEGATIVE_SIGN_FIELD = new EpochField(
Expand Down Expand Up @@ -161,6 +180,10 @@ public TemporalAccessor resolve(
Long nanosOfMilli = fieldValues.remove(NANOS_OF_MILLI);
long secondsAndMillis = fieldValues.remove(this);

// piggyback whether the formatter is in a "round up" mode or not via negative nanos
// ... which are not valid except in the context of our parser wanting to round up
boolean roundUp = fieldValues.remove(ROUNDED_SIGN_FIELD) != null;

long seconds;
long nanos;
if (isNegative != null) {
Expand All @@ -169,10 +192,15 @@ public TemporalAccessor resolve(
nanos = secondsAndMillis % 1000 * 1_000_000;
// `secondsAndMillis < 0` implies negative timestamp; so `nanos < 0`
if (nanosOfMilli != null) {
// aggregate fractional part of the input; subtract b/c `nanos < 0`
nanos -= nanosOfMilli;
if (roundUp) {
// these are not the nanos you think they are; these are "round up nanos" not the fractional part of the input
nanos += nanosOfMilli;
} else {
// aggregate fractional part of the input; subtract b/c `nanos < 0`
nanos -= nanosOfMilli;
}
}
if (nanos != 0) {
if (nanos < 0) {
// nanos must be positive. B/c the timestamp is represented by the
// (seconds, nanos) tuple, seconds moves 1s toward negative-infinity
// and nanos moves 1s toward positive-infinity
Expand Down Expand Up @@ -218,7 +246,8 @@ public long getFrom(TemporalAccessor temporal) {
};

// this supports seconds without any fraction
private static final DateTimeFormatter SECONDS_FORMATTER1 = new DateTimeFormatterBuilder().optionalStart()
private static final DateTimeFormatter SECONDS_FORMATTER1 = new DateTimeFormatterBuilder()
.optionalStart()
.appendText(NEGATIVE_SIGN_FIELD, Map.of(-1L, "-")) // field is only created in the presence of a '-' char.
.optionalEnd()
.appendValue(UNSIGNED_SECONDS, 1, 19, SignStyle.NOT_NEGATIVE)
Expand All @@ -228,45 +257,79 @@ public long getFrom(TemporalAccessor temporal) {
.toFormatter(Locale.ROOT);

// this supports seconds ending in dot
private static final DateTimeFormatter SECONDS_FORMATTER2 = new DateTimeFormatterBuilder().optionalStart()
private static final DateTimeFormatter SECONDS_FORMATTER2 = new DateTimeFormatterBuilder()
.optionalStart()
.appendText(NEGATIVE_SIGN_FIELD, Map.of(-1L, "-")) // field is only created in the presence of a '-' char.
.optionalEnd()
.appendValue(UNSIGNED_SECONDS, 1, 19, SignStyle.NOT_NEGATIVE)
.appendLiteral('.')
.toFormatter(Locale.ROOT);

// this supports milliseconds
public static final DateTimeFormatter MILLISECONDS_FORMATTER1 = new DateTimeFormatterBuilder().optionalStart()
static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter(
"epoch_second",
new JavaTimeDateTimePrinter(SECONDS_FORMATTER1),
JavaTimeDateTimeParser.createRoundUpParserGenerator(builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L)),
new JavaTimeDateTimeParser(SECONDS_FORMATTER1),
new JavaTimeDateTimeParser(SECONDS_FORMATTER2)
);

public static final DateTimeFormatter MILLISECONDS_FORMATTER_BASE = new DateTimeFormatterBuilder()
.optionalStart()
.appendText(NEGATIVE_SIGN_FIELD, Map.of(-1L, "-")) // field is only created in the presence of a '-' char.
.optionalEnd()
.appendValue(UNSIGNED_MILLIS, 1, 19, SignStyle.NOT_NEGATIVE)
.toFormatter(Locale.ROOT);


// FIXME: clean these up and append one to the other
// this supports milliseconds
public static final DateTimeFormatter MILLISECONDS_FORMATTER = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER_BASE)
.optionalStart()
.appendFraction(NANOS_OF_MILLI, 0, 6, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

// this supports milliseconds ending in dot
private static final DateTimeFormatter MILLISECONDS_FORMATTER2 = new DateTimeFormatterBuilder().optionalStart()
.appendText(NEGATIVE_SIGN_FIELD, Map.of(-1L, "-")) // field is only created in the presence of a '-' char.
// this supports milliseconds
public static final DateTimeFormatter MILLISECONDS_PARSER_W_NANOS = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER_BASE)
.appendFraction(NANOS_OF_MILLI, 0, 6, true)
.toFormatter(Locale.ROOT);

// we need an additional parser to detect the difference between user provided nanos and defaulted ones because of the necessity
// ... to parse the two differently in the round up case
public static final DateTimeFormatter MILLISECONDS_PARSER_WO_NANOS = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER_BASE)
.toFormatter(Locale.ROOT);

// we need an additional parser to detect the difference between user provided nanos and defaulted ones because of the necessity
// ... to parse the two differently in the round up case
public static final DateTimeFormatter MILLISECONDS_PARSER_WO_NANOS_ROUNDING = new DateTimeFormatterBuilder()
.optionalStart()
.appendText(ROUNDED_SIGN_FIELD, Map.of(-2L, "~"))
.optionalEnd()
.appendValue(UNSIGNED_MILLIS, 1, 19, SignStyle.NOT_NEGATIVE)
.appendLiteral('.')
.append(MILLISECONDS_FORMATTER_BASE)
.parseDefaulting(EpochTime.ROUNDED_SIGN_FIELD, -2L)
.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L)
.toFormatter(Locale.ROOT);

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter(
"epoch_second",
new JavaTimeDateTimePrinter(SECONDS_FORMATTER1),
JavaTimeDateTimeParser.createRoundUpParserGenerator(builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L)),
new JavaTimeDateTimeParser(SECONDS_FORMATTER1),
new JavaTimeDateTimeParser(SECONDS_FORMATTER2)
);
// this supports milliseconds ending in dot
private static final DateTimeFormatter MILLISECONDS_PARSER_ENDING_IN_PERIOD = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER_BASE)
.appendLiteral('.')
.toFormatter(Locale.ROOT);

static final DateFormatter MILLIS_FORMATTER = new JavaDateFormatter(
"epoch_millis",
new JavaTimeDateTimePrinter(MILLISECONDS_FORMATTER1),
JavaTimeDateTimeParser.createRoundUpParserGenerator(builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L)),
new JavaTimeDateTimeParser(MILLISECONDS_FORMATTER1),
new JavaTimeDateTimeParser(MILLISECONDS_FORMATTER2)
new JavaTimeDateTimePrinter(MILLISECONDS_FORMATTER),
new JavaTimeDateTimeParser[]{
new JavaTimeDateTimeParser(MILLISECONDS_PARSER_WO_NANOS_ROUNDING),
new JavaTimeDateTimeParser(MILLISECONDS_PARSER_W_NANOS),
new JavaTimeDateTimeParser(MILLISECONDS_PARSER_ENDING_IN_PERIOD)},
new JavaTimeDateTimeParser[]{
new JavaTimeDateTimeParser(MILLISECONDS_PARSER_WO_NANOS),
new JavaTimeDateTimeParser(MILLISECONDS_PARSER_W_NANOS),
new JavaTimeDateTimeParser(MILLISECONDS_PARSER_ENDING_IN_PERIOD)}
);

private abstract static class EpochField implements TemporalField {
Expand Down
29 changes: 26 additions & 3 deletions server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,33 @@ static DateFormatter combined(String input, List<DateFormatter> formatters) {
input,
printer,
roundUpParsers.stream().flatMap(Arrays::stream).toArray(DateTimeParser[]::new),
parsers.stream().flatMap(Arrays::stream).toArray(DateTimeParser[]::new)
parsers.stream().flatMap(Arrays::stream).toArray(DateTimeParser[]::new),
false
);
}

private JavaDateFormatter(String format, DateTimePrinter printer, DateTimeParser[] roundupParsers, DateTimeParser[] parsers) {
JavaDateFormatter(String format, DateTimePrinter printer, DateTimeParser[] roundupParsers, DateTimeParser[] parsers) {
this(format,
printer,
Arrays.copyOf(roundupParsers, roundupParsers.length, DateTimeParser[].class),
Arrays.copyOf(parsers, parsers.length, DateTimeParser[].class),
true);
}

private JavaDateFormatter(String format, DateTimePrinter printer, DateTimeParser[] roundupParsers, DateTimeParser[] parsers,
boolean doValidate) {
if(doValidate) {
if (format.contains("||")) {
throw new IllegalArgumentException("This class cannot handle multiple format specifiers");
}
if (printer == null) {
throw new IllegalArgumentException("printer may not be null");
}
if (parsers.length == 0) {
throw new IllegalArgumentException("parsers need to be specified");
}
verifyPrinterParsers(printer, parsers);
}
this.format = format;
this.printer = printer;
this.roundupParsers = roundupParsers;
Expand Down Expand Up @@ -247,7 +269,8 @@ private JavaDateFormatter mapParsers(UnaryOperator<DateTimePrinter> printerMappi
format,
printerMapping.apply(printer),
mapParsers(parserMapping, this.roundupParsers),
mapParsers(parserMapping, this.parsers)
mapParsers(parserMapping, this.parsers),
false
);
}

Expand Down
Loading

0 comments on commit 190bb7c

Please sign in to comment.