Handle long overflow in dates

[pulak777]

This makes sure that dates that overflow a long fail with a reasonable message rather than an ArithmeticException. This mostly just hits the field mapper.

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[nik9000]

@elasticmachine, ok to test

[elasticsearchmachine]

Pinging @elastic/es-search-foundations (Team:Search Foundations)

[nik9000]

I think it'd be nice to have a file like 80_date_nanos.yml that asserts what we expect to happen with this one on dates. Those files are the most integration-test-style things we do and is nice and paranoid about the presentation of the error.

[nik9000]

@elasticsearchmachine test this please

[nik9000]

I'm removing myself as the assignee. I think it should be one of the search foundations folks. I can certainly review the ESQL side of things.

[cla-checker-service]

💚 CLA has been signed

[smalyshev]

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

[pulak777]

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)

[javanna]

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?

[pulak777]

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

[smalyshev]

Yeah maybe I was confusing it with another exception.

[javanna]

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

[smalyshev]

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.

[javanna]

++ sounds good.

[smalyshev]

@elasticmachine test this please

[smalyshev]

@pulak777 looks like we have some checkstyle problems here, please fix them. Usually just running ./gradlew spotlessApply takes care of that.

[pulak777]

done

[smalyshev]

@elasticmachine test this please

[smalyshev]

Looks like it breaks some tests, e.g.:

DateFieldMapperTests > testIgnoreMalformedFalseByDefault FAILED
  | java.lang.AssertionError: incorrect exception while parsing {"field":"-522000000"}
  | Expected: a string containing "long overflow"
  | but: was "date[-522000000-01-01T00:00:00Z] is too far in the past to fit in a long milliseconds variable"

[pulak777]

is it possible to grant me access to see the failing tests on ci? it kinda takes forever to run all the tests on my laptop

[smalyshev]

@pulak777 Not sure about the access, but look at org.elasticsearch.index.mapper.DateFieldMapperTests there are a bunch of failing tests there. You should be able to just run ./gradlew ":server:test" --tests "org.elasticsearch.index.mapper.DateFieldMapperTests" and see it, it should be pretty quick.

[pulak777]

updated the failing tests in "org.elasticsearch.index.mapper.DateFieldMapperTests"

[smalyshev]

@elasticmachine test this please

[smalyshev]

@elasticmachine update branch

[smalyshev]

@elasticmachine test this please

[smalyshev]

@elasticmachine update branch

[smalyshev]

@elasticmachine test this please

[smalyshev]

Replaced by #124048 to update to current code.