[ESQL] copy ql disjunction rule into esql #106499
Conversation
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| List<Expression> ors = new LinkedList<>(); | ||
|
|
||
| for (Expression exp : exps) { | ||
| if (exp instanceof Equals eq) { |
There was a problem hiding this comment.
Note, the key change here is that this now refers to esql's Equals, not the ql base Equals.
There was a problem hiding this comment.
You could minimize the change by adding a templating method in the basic QL rule and overriding this in ESQL:
for (Expression exp : exps) {
if (isEquals(exp)) {
....
}
}
public boolean isEquals(Expression exp) {
return exp instanceof QlEquals;
}
}
// rule in ESQL
public CombineDisjunctionsIn... extends ql.CombineDisjunctions {
@Override
protected boolean isEquals(Expression ex) {
return ex instanceof EsqlEquals;
}
}
| @@ -1138,6 +1145,57 @@ protected In createIn(Expression key, List<Expression> values, ZoneId zoneId) { | |||
| protected Equals createEquals(Expression k, Set<Expression> v, ZoneId finalZoneId) { | |||
| return new Equals(k.source(), k, v.iterator().next(), finalZoneId); | |||
| } | |||
|
|
|||
| @Override | |||
| protected Expression rule(Or or) { | |||
There was a problem hiding this comment.
This method copied in from OptimizerRules
|
Looking at it, there's a lot of tests in |
| * This rule does NOT check for type compatibility as that phase has been | ||
| * already be verified in the analyzer. | ||
| */ | ||
| public static class CombineDisjunctionsToIn extends OptimizerRules.CombineDisjunctionsToIn { |
There was a problem hiding this comment.
Moved to OptimizerRules (ESQL version) to mirror the QL class hierarchy
| } | ||
|
|
||
| @Override | ||
| protected Expression rule(Or or) { |
There was a problem hiding this comment.
Rule logic copied from QL version, but now pointing to ESQL versions of the classes
| @@ -3441,15 +3475,6 @@ private void assertNullLiteral(Expression expression) { | |||
| assertNull(expression.fold()); | |||
| } | |||
|
|
|||
| // TODO: move these from org.elasticsearch.xpack.ql.optimizer.OptimizerRulesTests to org.elasticsearch.xpack.ql.TestUtils | |||
|
Actually, I did port the tests when I did the class hierarchy revision you asked for. They're in the ESQL version of |
Relates to #105217 This copies the PropagateEquals logical optimization into ESQL, following the pattern established in #106499. I've copied the optimization rule into the ESQL version of OptimizerRules, and the tests into OpitmizerRulesTests, and changed the imports &c to point to the appropriate ESQL classes instead of their QL counterparts. I expect to have several more PRs following this pattern, for the remaining logical optimizations that touch the binary comparison logic. I'm intending to make separate PRs for each, in the interest of making them easier to review.
Relates to #105217 This copies the BooleanFunctionEqualsElimination logical optimization into ESQL, following the pattern established in #106499. I've copied the optimization rule into the ESQL version of OptimizerRules, and the tests into OpitmizerRulesTests, and changed the imports &c to point to the appropriate ESQL classes instead of their QL counterparts. I only saw two tests for this one.
Similar to #105711, this migrates a QL optimization that depends on specific class types into ESQL to refer to the ESQL version of those classes. This is necessary for #105217, which changes the class hierarchy of the binary comparisons.
In this case, I didn't find any tests that exercise this optimization directly, although a failure in
PhysicalPlanOptimizerTestsalerted me to the problem. To that end, I've added three tests to directly cover this optimization.From the look of it, there's several more logical planner level optimizations that rely on the exact type of the comparison classes, so I'm going to migrate those as well. For ease of review, I figured I'd do those in separate PRs.