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 #2 #119378

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

Conversation

fang-xing-esql
Copy link
Member

This is an alternative approach to wrap constant folding errors into VerificationExceptions, instead of returning warnings + nulls.

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

The first approach is here

Previously, the warnings/exceptions created by ExpressionEvaluator are stored in thread_context, and they are not returned to planner after constant folding, so they are not visible to planner. This PR adds a method evalFoldable that throw Exceptions for errors happen during the evaluation of foldable expressions in ExpressionEvaluator, and it is called by EvaluatorMapper.fold() during LogicalPlanOptimizer. evalFoldable does not create warnings and add them into thread_context, it throws the Exception when there is error happens directly, later on these Exceptions are wrapped into VerificationExceptions by EvaluatorMapper.fold(), ConstantFolding and PropagateEvalFoldables.

Conversion functions are not covered by this PR yet. There are some examples in the CsvTests that are referenced by docs.

@elasticsearchmachine
Copy link
Collaborator

Hi @fang-xing-esql, I've created a changelog YAML for you.

* This is called by {@code EvaluatorMapper.fold()}, {@code ConstantFolding} and {@code PropagateEvalFoldables}.
* {@code EvaluatorMapper.fold()} is not aware of the location, null is provided for location.
*/
public static VerificationException convertToVerificationException(Exception e, Location location) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling this method in ConstantFolding and PropagateEvalFoldables have you thought about calling it in Literal.of() when the fold() method is called on the Literal constructor?

@astefan astefan requested a review from costin January 9, 2025 14:55
@astefan
Copy link
Contributor

astefan commented Jan 9, 2025

Foldables is also calling fold on a foldable expression... Off the top of my head, I don't remember why we have that. It seems there are some special cases for in and some geo-related logic that need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants