elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
69.64k stars 24.64k forks source link

ESQL: Improve error handling when folding constants #99758

Open alex-spies opened 1 year ago

alex-spies commented 1 year ago

Description

When folding constants before a query is properly evaluated, date math arithmetic exceptions result in an error message in the REST response whereas other arithmetic exceptions only result in warnings.

We should treat errors during constant folding consistently.

For instance:

{"query": "row n = now() | eval n + (2147483647 year + 1 year)"}

results in a REST response containing

{"type":"illegal_temporal_value_exception","reason":"arithmetic exception in expression [2147483647 year + 1 year]: [integer overflow]"}

and status code 400.

But for

{"query": "row n = 1 | eval n + (2147483647 + 1)"}

one obtains

       n       |n+(2147483647+1)
---------------+----------------
1              |null      

and a warning header:

Warning: 299 Elasticsearch-8.11.0-SNAPSHOT-b6747b48ba0901a530ba51375e3576fdb8df8527 "Line 1:23: evaluation of [2147483647 + 1] failed, treating result as null. Only first 20 failures recorded."

In particular, the first query never hits the compute service, but the second does - always evaluating the expression to null.

Since exceptions during constant folding mean that the whole query will be essentially pointless (the offending expression will always return null and waste potentially a lot of compute time), IMO it's preferable to consistently send back a 400 REST response in such cases instead of warnings.

elasticsearchmachine commented 1 year ago

Pinging @elastic/es-ql (Team:QL)

alex-spies commented 1 year ago

For some background: DateTimeArithmeticOperator::fold uses the default fold implementation which applies an evaluator like during normal expression evaluation outside of folding. Normal evaluation, however, catches arithmetic exceptions and turns them into warnings (which IMO is correct as we wouldn't want to stop execution with an error just because there was e.g. one overflow).

bpintea commented 11 months ago

IMO it's preferable to consistently send back a 400 REST response in such cases instead of warnings.

It is. This noted issue happens because we don't fold as much as we could when optimising and leave the rest to be executed (#99502).

alex-spies commented 11 months ago

IMO it's preferable to consistently send back a 400 REST response in such cases instead of warnings.

It is. This noted issue happens because we don't fold as much as we could when optimising and leave the rest to be executed (#99502).

I think arithmetic expressions are actually folded before execution just fine (should be done in the constant folding optimization rule) - but using the standard evaluators, which record warnings + return null. So it looks like it was just not folded.

elasticsearchmachine commented 11 months ago

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

elasticsearchmachine commented 8 months ago

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine commented 5 months ago

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