Skip to content

Handle long overflow in dates #112650

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

Closed
wants to merge 9 commits into from
Closed

Handle long overflow in dates #112650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3caf86b
Handle long overflow exception in dateFieldMapper component #112483
pulak777 Sep 9, 2024
0c9b57f
Update docs/changelog/112650.yaml
pulak777 Sep 9, 2024
fe1655d
revert before epoch rejections
pulak777 Sep 9, 2024
0dcf952
Fix typos
pulak777 Sep 10, 2024
c263056
Merge branch 'main' into handle-long-overflow
elasticmachine Sep 26, 2024
f66ecf2
fix formatting
pulak777 Sep 26, 2024
d17ebc0
fix failing tests
pulak777 Sep 30, 2024
e762144
Merge branch 'main' into handle-long-overflow
elasticmachine Oct 3, 2024
21cb92b
Merge branch 'main' into handle-long-overflow
elasticmachine Oct 9, 2024
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
6 changes: 6 additions & 0 deletions docs/changelog/112650.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 112650
summary: Handle ArithmeticException(long overflow) in dateFieldMapper
area: Search
type: bug
issues:
- 112483
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this patch solves issue #112483 though - wouldn't IllegalArgumentException still produce 500? One would have to catch it in DateFieldMapper and convert it to ElasticsearchParseException to make it 4xx if I understand it correctly. Which however raises the question - if we'd have to catch it there, does it really need to be also converted in toLongMillis (not a rhetorical question, I don't have a predetermined answer to it)?

Copy link
Author

@pulak777 pulak777 Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I totally agree with this point. Now I think, the only use case of this patch can be providing better error message to the user (given that we catch it in DateFieldMapper as well, and convert to ElasticsearchParseException to make it 4xx)

Copy link
Member

Choose a reason for hiding this comment

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

IllegalArgumentException does normally get converted to a 400 error here: https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/ExceptionsHelper.java#L64 . Any reason why this one specifically would lead to a 500 instead?

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't aware of this and assumed IllegalArgumentException returns 500 based on previous comment. As that's not the case, I think the patch should work without additonal modification

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah maybe I was confusing it with another exception.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test that verifies this? Don't just believe me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't verify the full circle here because the original issue did not contain reproducing example and I am not sure how to reproduce it from scratch. But this suggests @javanna is correct:

elasticsearch/server/src/main/java/org/elasticsearch/ExceptionsHelper.java

Line 63 in f412de7

} else if (t instanceof IllegalArgumentException) {
so while I'd be more happy if we had full cycle test I think this is ok.

Copy link
Member

Choose a reason for hiding this comment

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

++ sounds good.

19 changes: 19 additions & 0 deletions server/src/main/java/org/elasticsearch/common/time/DateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,25 @@ public static long toLong(Instant instant) {
return instant.getEpochSecond() * 1_000_000_000 + instant.getNano();
}

/**
* Convert a java time instant to a long value which is stored in lucene,
* the long value resembles the milliseconds since the epoch
*
* @param instant the instant to convert
* @return the total milliseconds as a single long
*/
public static long toLongMillis(Instant instant) {
try {
return instant.toEpochMilli();
} catch (ArithmeticException e) {
if (instant.isAfter(Instant.now())) {
throw new IllegalArgumentException("date[" + instant + "] is too far in the future to fit in a long milliseconds variable");
} else {
throw new IllegalArgumentException("date[" + instant + "] is too far in the past to fit in a long milliseconds variable");
}
}
}

/**
* Returns an instant that is with valid nanosecond resolution. If
* the parameter is before the valid nanosecond range then this returns
Expand Down
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 @@ -71,6 +71,7 @@
import java.util.function.LongSupplier;

import static org.elasticsearch.common.time.DateUtils.toLong;
import static org.elasticsearch.common.time.DateUtils.toLongMillis;

/** A {@link FieldMapper} for dates. */
public final class DateFieldMapper extends FieldMapper {
Expand All @@ -94,7 +95,7 @@ public enum Resolution {
MILLISECONDS(CONTENT_TYPE, NumericType.DATE, DateMillisDocValuesField::new) {
@Override
public long convert(Instant instant) {
return instant.toEpochMilli();
return toLongMillis(instant);
}

@Override
Expand Down
23 changes: 23 additions & 0 deletions server/src/test/java/org/elasticsearch/common/time/DateUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.elasticsearch.common.time.DateUtils.clampToNanosRange;
import static org.elasticsearch.common.time.DateUtils.toInstant;
import static org.elasticsearch.common.time.DateUtils.toLong;
import static org.elasticsearch.common.time.DateUtils.toLongMillis;
import static org.elasticsearch.common.time.DateUtils.toMilliSeconds;
import static org.elasticsearch.common.time.DateUtils.toNanoSeconds;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -53,6 +54,28 @@ public void testInstantToLongMax() {
assertThat(e.getMessage(), containsString("is after"));
}

public void testInstantToLongMillis() {
assertThat(toLongMillis(Instant.EPOCH), is(0L));

Instant instant = createRandomInstant();
long timeSinceEpochInMillis = instant.toEpochMilli();
assertThat(toLongMillis(instant), is(timeSinceEpochInMillis));
}

public void testInstantToLongMillisMin() {
/* negative millisecond value of this instant exceeds the maximum value a java long variable can store */
Instant tooEarlyInstant = Instant.ofEpochSecond(-9223372036854776L);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> toLongMillis(tooEarlyInstant));
assertThat(e.getMessage(), containsString("too far in the past"));
}

public void testInstantToLongMillisMax() {
/* millisecond value of this instant exceeds the maximum value a java long variable can store */
Instant tooLateInstant = Instant.ofEpochSecond(9223372036854776L);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> toLongMillis(tooLateInstant));
assertThat(e.getMessage(), containsString("too far in the future"));
}

public void testLongToInstant() {
assertThat(toInstant(0), is(Instant.EPOCH));
assertThat(toInstant(1), is(Instant.EPOCH.plusNanos(1)));
Expand Down
5 changes: 4 additions & 1 deletion 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 @@ -149,7 +149,10 @@ protected List<ExampleMalformedValue> exampleMalformedValues() {
return List.of(
exampleMalformedValue("2016-03-99").mapping(mappingWithFormat("strict_date_optional_time||epoch_millis"))
.errorMatches("failed to parse date field [2016-03-99] with format [strict_date_optional_time||epoch_millis]"),
exampleMalformedValue("-522000000").mapping(mappingWithFormat("date_optional_time")).errorMatches("long overflow"),
exampleMalformedValue("-522000000").mapping(mappingWithFormat("date_optional_time"))
.errorMatches("too far in the past to fit in a long"),
exampleMalformedValue("522000000").mapping(mappingWithFormat("date_optional_time"))
.errorMatches("too far in the future to fit in a long"),
exampleMalformedValue("2020").mapping(mappingWithFormat("strict_date"))
.errorMatches("failed to parse date field [2020] with format [strict_date]"),
exampleMalformedValue("hello world").mapping(mappingWithFormat("strict_date_optional_time"))
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDatetimeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public static Iterable<Object[]> parameters() {
suppliers,
"ToDatetimeFromDateNanosEvaluator[field=" + read + "]",
DataType.DATETIME,
i -> DateUtils.toMilliSeconds(DateUtils.toLong(i)),
DateUtils::toLongMillis,
emptyList()
);

Expand Down
3 changes: 1 addition & 2 deletions ...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToLongTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;

import java.math.BigInteger;
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Function;
Expand All @@ -43,7 +42,7 @@ public static Iterable<Object[]> parameters() {
TestCaseSupplier.forUnaryBoolean(suppliers, evaluatorName.apply("Boolean"), DataType.LONG, b -> b ? 1L : 0L, List.of());

// datetimes
TestCaseSupplier.forUnaryDatetime(suppliers, read, DataType.LONG, Instant::toEpochMilli, List.of());
TestCaseSupplier.forUnaryDatetime(suppliers, read, DataType.LONG, DateUtils::toLongMillis, List.of());
TestCaseSupplier.forUnaryDateNanos(suppliers, read, DataType.LONG, DateUtils::toLong, List.of());
// random strings that don't look like a long
TestCaseSupplier.forUnaryStrings(
Expand Down
2 changes: 1 addition & 1 deletion ...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToStringTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public static Iterable<Object[]> parameters() {
suppliers,
"ToStringFromDatetimeEvaluator[field=" + read + "]",
DataType.KEYWORD,
i -> new BytesRef(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.formatMillis(i.toEpochMilli())),
i -> new BytesRef(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.formatMillis(DateUtils.toLongMillis(i))),
List.of()
);
TestCaseSupplier.forUnaryDateNanos(
Expand Down