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

support negative epoch_millis timestamps #80208

Merged
merged 12 commits into from
Mar 24, 2022
Merged

support negative epoch_millis timestamps #80208

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6fce9cd
support negative epoch_timestamps
Nov 2, 2021
a30f83a
Merge branch 'master' into support_negative_epoch_millis_timestamps
elasticmachine Feb 9, 2022
c699886
support negative fractional epochs where seconds is 0.
Feb 20, 2022
c849b93
Merge branch 'support_negative_epoch_millis_timestamps' of https://gi…
Feb 20, 2022
f225fcf
checkstyle - replace "!" with explicit "== false"
Feb 20, 2022
96aeb31
avoid using Math.abs
Feb 21, 2022
926bc49
Merge branch 'master' into support_negative_epoch_millis_timestamps
elasticmachine Feb 21, 2022
7919af4
comment on negative epoch rounding; remove unnecessary code
Mar 21, 2022
bcf2293
support negative epochs for epoch_seconds
Mar 21, 2022
dda79f3
Merge branch 'master' into support_negative_epoch_millis_timestamps
elasticmachine Mar 23, 2022
9cbae7f
add changelog
rjernst Mar 23, 2022
66d1ec6
fix changelog format
rjernst Mar 23, 2022
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
106 changes: 87 additions & 19 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 @@ -28,13 +28,14 @@
* The seconds formatter is provided by {@link #SECONDS_FORMATTER}.
* The milliseconds formatter is provided by {@link #MILLIS_FORMATTER}.
* <p>
* Both formatters support fractional time, up to nanosecond precision. Values must be positive numbers.
* Both formatters support fractional time, up to nanosecond precision.
*/
class EpochTime {

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

private static final EpochField SECONDS = new EpochField(ChronoUnit.SECONDS, ChronoUnit.FOREVER, LONG_POSITIVE_RANGE) {
private static final EpochField SECONDS = new EpochField(ChronoUnit.SECONDS, ChronoUnit.FOREVER, LONG_INTEGER_RANGE) {
@Override
public boolean isSupportedBy(TemporalAccessor temporal) {
return temporal.isSupported(ChronoField.INSTANT_SECONDS);
Expand All @@ -61,6 +62,24 @@ public TemporalAccessor resolve(
}
};

// 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(
ChronoUnit.FOREVER,
ChronoUnit.FOREVER,
ValueRange.of(NEGATIVE_SIGN_PLACEHOLDER, NEGATIVE_SIGN_PLACEHOLDER)
) {
@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 NEGATIVE_SIGN_PLACEHOLDER;
}
};

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) {
Expand All @@ -73,15 +92,30 @@ public long getFrom(TemporalAccessor temporal) {
}
};

private static final EpochField MILLIS = new EpochField(ChronoUnit.MILLIS, ChronoUnit.FOREVER, LONG_POSITIVE_RANGE) {
private static final EpochField UNSIGNED_MILLIS = new EpochField(ChronoUnit.MILLIS, ChronoUnit.FOREVER, POSITIVE_LONG_INTEGER_RANGE) {
@Override
public boolean isSupportedBy(TemporalAccessor temporal) {
return temporal.isSupported(ChronoField.INSTANT_SECONDS) && temporal.isSupported(ChronoField.MILLI_OF_SECOND);
return temporal.isSupported(ChronoField.INSTANT_SECONDS)
&& (temporal.isSupported(ChronoField.NANO_OF_SECOND) || temporal.isSupported(ChronoField.MILLI_OF_SECOND));
}

@Override
public long getFrom(TemporalAccessor temporal) {
return temporal.getLong(ChronoField.INSTANT_SECONDS) * 1_000 + temporal.getLong(ChronoField.MILLI_OF_SECOND);
long millis = temporal.getLong(ChronoField.INSTANT_SECONDS) * 1_000;
if (millis >= 0 || temporal.isSupported(ChronoField.NANO_OF_SECOND) == false) {
return millis + temporal.getLong(ChronoField.MILLI_OF_SECOND);
} else {
long nanos = temporal.getLong(ChronoField.NANO_OF_SECOND);
if (nanos % 1_000_000 != 0) {
// Fractional negative timestamp.
// Add 1 ms towards positive infinity because the fraction leads
Copy link
Member

Choose a reason for hiding this comment

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

This adjustment is tricky, and I had to stare at it for a while to understand. I think the comment would be more helpful if it mentions rounding. Essentially there is some extra precision that we are going to throw away by converting this to millis, and this adjustment by 1 ensures we are always rounding towards the epoch. Is that right? If so, can you improve the comment a bit?

Copy link
Author

@rkophs rkophs Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Your understanding is correct. If we get into this branch, it means two things: 1) we're dealing with a negative epoch, 2) we're dealing with a fractional millisecond. Thus, the calculated millis invokes a precision loss that needs to be rounded towards the epoch for formatting (i.e. to a smaller magnitude for formatting, since Java date-time stores it as a negative at a higher magnitude by 1)

I'll update the comment to reflect.

// the output's integral part to be an off-by-one when the
// `(nanos / 1_000_000)` is added below.
millis += 1;
}
millis += (nanos / 1_000_000);
return -millis; // positive for formatting; sign handled by NEGATIVE_SIGN_FIELD
}
}

@Override
Expand All @@ -90,19 +124,47 @@ public TemporalAccessor resolve(
TemporalAccessor partialTemporal,
ResolverStyle resolverStyle
) {
long secondsAndMillis = fieldValues.remove(this);
long seconds = secondsAndMillis / 1_000;
long nanos = secondsAndMillis % 1000 * 1_000_000;
Long isNegative = fieldValues.remove(NEGATIVE_SIGN_FIELD);
Long nanosOfMilli = fieldValues.remove(NANOS_OF_MILLI);
if (nanosOfMilli != null) {
nanos += nanosOfMilli;
long secondsAndMillis = fieldValues.remove(this);

long seconds;
long nanos;
if (isNegative != null) {
secondsAndMillis = -secondsAndMillis;
seconds = secondsAndMillis / 1_000;
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 (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
seconds -= 1;
nanos = 1_000_000_000 + nanos;
}
} else {
seconds = secondsAndMillis / 1_000;
nanos = secondsAndMillis % 1000 * 1_000_000;

if (nanosOfMilli != null) {
// aggregate fractional part of the input
nanos += nanosOfMilli;
}
}

fieldValues.put(ChronoField.INSTANT_SECONDS, seconds);
fieldValues.put(ChronoField.NANO_OF_SECOND, nanos);
// if there is already a milli of second, we need to overwrite it
if (fieldValues.containsKey(ChronoField.MILLI_OF_SECOND)) {
fieldValues.put(ChronoField.MILLI_OF_SECOND, nanos / 1_000_000);
}
if (fieldValues.containsKey(ChronoField.MICRO_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.

Why is this needed?

Copy link
Author

@rkophs rkophs Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is not needed. I added it defensively since the same logic is happening above with MILLI_OF_SECOND, but I'm happy to remove it since it's not directly related to the change at hand.

fieldValues.put(ChronoField.MICRO_OF_SECOND, nanos / 1_000);
}
return null;
}
};
Expand All @@ -117,7 +179,11 @@ public boolean isSupportedBy(TemporalAccessor temporal) {

@Override
public long getFrom(TemporalAccessor temporal) {
return temporal.getLong(ChronoField.NANO_OF_SECOND) % 1_000_000;
if (temporal.getLong(ChronoField.INSTANT_SECONDS) < 0) {
return (1_000_000_000 - temporal.getLong(ChronoField.NANO_OF_SECOND)) % 1_000_000;
} else {
return temporal.getLong(ChronoField.NANO_OF_SECOND) % 1_000_000;
}
}
};

Expand All @@ -133,13 +199,15 @@ public long getFrom(TemporalAccessor temporal) {
.appendLiteral('.')
.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
public static final DateTimeFormatter MILLISECONDS_FORMATTER1 = 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)
.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().append(MILLISECONDS_FORMATTER1)
Expand Down
162 changes: 161 additions & 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 @@ -92,16 +92,120 @@ public void testEpochMillisParser() {
Instant instant = Instant.from(formatter.parse("12345"));
assertThat(instant.getEpochSecond(), is(12L));
assertThat(instant.getNano(), is(345_000_000));
assertThat(formatter.format(instant), is("12345"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("0"));
assertThat(instant.getEpochSecond(), is(0L));
assertThat(instant.getNano(), is(0));
assertThat(formatter.format(instant), is("0"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("123.123456"));
assertThat(instant.getEpochSecond(), is(0L));
assertThat(instant.getNano(), is(123123456));
assertThat(formatter.format(instant), is("123.123456"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-123.123456"));
assertThat(instant.getEpochSecond(), is(-1L));
assertThat(instant.getNano(), is(876876544));
assertThat(formatter.format(instant), is("-123.123456"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6789123.123456"));
assertThat(instant.getEpochSecond(), is(-6790L));
assertThat(instant.getNano(), is(876876544));
assertThat(formatter.format(instant), is("-6789123.123456"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("6789123.123456"));
assertThat(instant.getEpochSecond(), is(6789L));
assertThat(instant.getNano(), is(123123456));
assertThat(formatter.format(instant), is("6789123.123456"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000430768.25"));
assertThat(instant.getEpochSecond(), is(-6250000431L));
assertThat(instant.getNano(), is(231750000));
assertThat(formatter.format(instant), is("-6250000430768.25"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000430768.75"));
assertThat(instant.getEpochSecond(), is(-6250000431L));
assertThat(instant.getNano(), is(231250000));
assertThat(formatter.format(instant), is("-6250000430768.75"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000430768.00"));
assertThat(instant.getEpochSecond(), is(-6250000431L));
assertThat(instant.getNano(), is(232000000));
assertThat(formatter.format(instant), is("-6250000430768")); // remove .00 precision
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000431000.250000"));
assertThat(instant.getEpochSecond(), is(-6250000432L));
assertThat(instant.getNano(), is(999750000));
assertThat(formatter.format(instant), is("-6250000431000.25"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000431000.000001"));
assertThat(instant.getEpochSecond(), is(-6250000432L));
assertThat(instant.getNano(), is(999999999));
assertThat(formatter.format(instant), is("-6250000431000.000001"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000431000.75"));
assertThat(instant.getEpochSecond(), is(-6250000432L));
assertThat(instant.getNano(), is(999250000));
assertThat(formatter.format(instant), is("-6250000431000.75"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000431000.00"));
assertThat(instant.getEpochSecond(), is(-6250000431L));
assertThat(instant.getNano(), is(0));
assertThat(formatter.format(instant), is("-6250000431000"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000431000"));
assertThat(instant.getEpochSecond(), is(-6250000431L));
assertThat(instant.getNano(), is(0));
assertThat(formatter.format(instant), is("-6250000431000"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-6250000430768"));
assertThat(instant.getEpochSecond(), is(-6250000431L));
assertThat(instant.getNano(), is(232000000));
assertThat(formatter.format(instant), is("-6250000430768"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("1680000430768"));
assertThat(instant.getEpochSecond(), is(1680000430L));
assertThat(instant.getNano(), is(768000000));
assertThat(formatter.format(instant), is("1680000430768"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("-0.12345"));
assertThat(instant.getEpochSecond(), is(-1L));
assertThat(instant.getNano(), is(999876550));
assertThat(formatter.format(instant), is("-0.12345"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
}

Expand Down Expand Up @@ -229,7 +333,7 @@ public void testSupportBackwardsJava8Format() {
assertThat(formatter, instanceOf(JavaDateFormatter.class));
}

public void testEpochFormatting() {
public void testEpochFormattingPositiveEpoch() {
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);
Expand All @@ -249,6 +353,62 @@ public void testEpochFormatting() {
assertThat(secondsFormatter.format(Instant.ofEpochSecond(42, 0)), is("42"));
}

public void testEpochFormattingNegativeEpoch() {
long seconds = randomLongBetween(-130L * 365 * 86400, 0); // around 1840 till 1970 epoch
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("-41876.543211"));

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 testEpochAndIso8601RoundTripNegative() {
long seconds = randomLongBetween(-130L * 365 * 86400, 0); // around 1840 till 1970 epoch
long nanos = randomLongBetween(0, 999_999_999L);
Instant instant = Instant.ofEpochSecond(seconds, nanos);

DateFormatter isoFormatter = DateFormatters.forPattern("strict_date_optional_time_nanos");
DateFormatter millisFormatter = DateFormatter.forPattern("epoch_millis");
String millis = millisFormatter.format(instant);
String iso8601 = isoFormatter.format(instant);

Instant millisInstant = Instant.from(millisFormatter.parse(millis));
Instant isoInstant = Instant.from(isoFormatter.parse(iso8601));

assertThat(millisInstant.toEpochMilli(), is(isoInstant.toEpochMilli()));
assertThat(millisInstant.getEpochSecond(), is(isoInstant.getEpochSecond()));
assertThat(millisInstant.getNano(), is(isoInstant.getNano()));
}

public void testEpochAndIso8601RoundTripPositive() {
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 isoFormatter = DateFormatters.forPattern("strict_date_optional_time_nanos");
DateFormatter millisFormatter = DateFormatter.forPattern("epoch_millis");
String millis = millisFormatter.format(instant);
String iso8601 = isoFormatter.format(instant);

Instant millisInstant = Instant.from(millisFormatter.parse(millis));
Instant isoInstant = Instant.from(isoFormatter.parse(iso8601));

assertThat(millisInstant.toEpochMilli(), is(isoInstant.toEpochMilli()));
assertThat(millisInstant.getEpochSecond(), is(isoInstant.getEpochSecond()));
assertThat(millisInstant.getNano(), is(isoInstant.getNano()));
}

public void testParsingStrictNanoDates() {
DateFormatter formatter = DateFormatters.forPattern("strict_date_optional_time_nanos");
formatter.format(formatter.parse("2016-01-01T00:00:00.000"));
Expand Down
5 changes: 0 additions & 5 deletions server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,6 @@ public void testIgnoreMalformed() throws IOException {
"failed to parse date field [2016-03-99] with format [strict_date_optional_time||epoch_millis]",
"strict_date_optional_time||epoch_millis"
);
testIgnoreMalformedForValue(
"-2147483648",
"failed to parse date field [-2147483648] with format [strict_date_optional_time||epoch_millis]",
"strict_date_optional_time||epoch_millis"
);
testIgnoreMalformedForValue("-522000000", "long overflow", "date_optional_time");
}

Expand Down
4 changes: 2 additions & 2 deletions server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ public void testParseEpochSecondsTimezone() {
zone,
Resolution.MILLISECONDS
);
long millis = randomNonNegativeLong();
long millis = randomLong();
// Convert to seconds
millis -= (millis % 1000);
assertEquals(
Expand All @@ -284,7 +284,7 @@ public void testParseEpochMillisTimezone() {
zone,
Resolution.MILLISECONDS
);
long millis = randomNonNegativeLong();
long millis = randomLong();
assertEquals(
"failed formatting for tz " + zone,
millis,
Expand Down