-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Fix issue with folding of CASE/IIF #49449
Conversation
Add extra checks to prevent ConstantFolding rule to try to fold the CASE/IIF functions early before the SimplifyCase rule gets applied. Fixes: elastic#49387
Pinging @elastic/es-search (:Search/SQL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider it a semantic leak when expression classes become aware of runtime rules.
It indicates that either the expression tries to do too much (in this case folding) or that the relationship between the rules is not correct (either the rules need to be simplified or the order between them changed).
The comment refers to a rule but it seems to me that the folding is eager?
Or maybe SimplifyCase
be moved before ConstantFolding
- I couldn't tell from the test case what is the current issue...
@costin The issue was that the Case was answering I thought of changing the order but I personally believe this approach can become easier a future confusion than my current solution. The current code just tests that the result or elseResult (depending on the TRUE/FALSE of the condition) is also foldable and (at least in my mind) doesn't appear that connected to the rules and their order. On the contrary the previous state seems to depend more on the ordering of the rules in the Optimizer, and the comment on the method (comes from a previous suggestion of yours) clearly states that it's based on the fact that the SimplifyCase rule has been applied. If you think changing the orders of the rules in the Optimizer seems a better approach, I'm happy to proceed with that. |
@costin
(please check the comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - the comment about the rules threw me off hence why it should be removed.
return true; | ||
} | ||
if (conditions.size() == 1 && conditions.get(0).condition().foldable()) { | ||
// Need to prevent ConstantFolding rule to get applied before SimplifyCase rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the comment misleading as it implies the rules order is incorrect - called in a different order the rules might affect fold
but the matter is, foldable
is incorrect (which leads to the exception).
@elasticmachine run elasticsearch-ci/default-distro |
@elasticmachine run elasticsearch-ci/packaging-sample-matrix |
Add extra checks to prevent ConstantFolding rule to try to fold
the CASE/IIF functions early before the SimplifyCase rule gets applied.
Fixes: #49387