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] Make EvaluatorMapper.fold() throw VerificationException for folding time errors Option#1 #116861

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

fang-xing-esql
Copy link
Member

@fang-xing-esql fang-xing-esql commented Nov 15, 2024

Resolves: #112672
Resolves: #99758
Resolves: #108519

This PR is working in progress, and the goal is to wrap the exceptions caught at constant folding(planning time errors) into VerificationException, in stead of returning null + warning. And this is a breaking change.

It differentiates user errors from data errors, if the literal values provided in the query text is wrong, the errors should be caught at planning time as VerificationException, if the data coming from indices are wrong, null + warnings could be returned at execution time.

Previously, the warnings/exceptions created by ExpressionEvaluator are stored in thread_context, and they are not returned to planner after constant folding - EvaluatorMapper.fold(), so they are not visible to planner. This PR redirects the warnings/exceptions caught by ExpressionEvaluator during constant folding to DriverContext, and planner can retrieve the warnings/exceptions from DriverContext, and wrap them into VerificationException.

I'd like to request an early review of this approach to see if it is in the right direction.

  • Conversion functions and case haven't been covered by this PR yet, they will be covered in this PR.
  • There are some other types of exceptions that planner can throw, e.g. InvalidArgumentException, IllegalArgumentException etc. when validating the literal values coded in the query, they are not wrapped into VerificationException by this PR yet. It is doable however it is another kind of breaking change, the client needs to capture a different type of exception, we can decide whether to do it or not, and if we decide to do it, whether to do it in the same or a separate PR.

@elasticsearchmachine
Copy link
Collaborator

Hi @fang-xing-esql, I've created a changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

@elasticsearchmachine
Copy link
Collaborator

Hi @fang-xing-esql, I've updated the changelog YAML for you.

@fang-xing-esql fang-xing-esql changed the title [ES|QL] Make EvaluatorMapper.fold() throw VerificationException for folding time errors [ES|QL] Make EvaluatorMapper.fold() throw VerificationException for folding time errors Option#1 Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants