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.

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

ES|QL: properly handle fold-time errors as VerificationExceptions #112672

Open
astefan opened this issue Sep 9, 2024 · 4 comments · May be fixed by #116861 or #119378
Open

ES|QL: properly handle fold-time errors as VerificationExceptions #112672

astefan opened this issue Sep 9, 2024 · 4 comments · May be fixed by #116861 or #119378
Assignees
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@astefan
Copy link
Contributor

astefan commented Sep 9, 2024

Description

A recent PR surfaced an inconsistency issue regarding functions that act on foldable values that are incorrect. For those functions the fold() method is called by the EvaluatorMapper and there are few aspects that can be improved:

  • for foldable values we shouldn't let the compute engine ignore them (and treat the resulting values as null and add a warning in the headers). Those foldable values errors should be reported to the user (the query is basically incorrect) as VerificationExceptions
  • establish a pattern/rule regarding the thrown exception for foldable values that don't meet functions' criteria. For example, the replace function throws a PatternSyntaxException when the regex is incorrect. This one should be replaced with IllegalArgumentException instead. If this syntax error comes from a foldable value, the IllegalArgumentException should be wrapped in a VerificationException (the exception being caught in EvaluatorMapper.fold()).
    • a VerificationException should always include the line and column where the error occured
    • once this exception pattern has been established our javadoc around contributing functions to ES|QL should be updated to reflect the desired behavior
    • the javadoc of the fold() method should also include this information
  • mathematical operators have the same behavior. Another related one, but slightly different in the light of recent improvements in the same area is here.
  • see if we can do better than dealing with these exceptions (and wrapping them in VerificationExceptions in EvaluatorMapper.fold()). The downside of doing it in the EvaluatorMappper.fold() is that there will be only one error message reported, unlike LogicalVerifier.verify() which collects and report all of them.
@astefan astefan self-assigned this Sep 9, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 9, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@alex-spies
Copy link
Contributor

Heya, there's already a bit of discussion on #99758, which I think this is a duplicate of.

@astefan astefan assigned fang-xing-esql and unassigned astefan Sep 10, 2024
@astefan
Copy link
Contributor Author

astefan commented Sep 10, 2024

@alex-spies thank you for chiming in, I didn't search for an already existent issue, there are some of them around indeed.
I've reworded a bit and extended the description of this one, hopefully this acts as a meta issue for all the rest. ES|QL needs a wide swipe of all functions and operators so that we have a consistent handling of this scenario.

@astefan astefan added v8.16.0 and removed v8.16.0 labels Sep 10, 2024
@fang-xing-esql
Copy link
Member

#108519 is also a variation of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment