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

[ESQL] Migrate PropagateEquals optimization #106627

Conversation

not-napoleon
Copy link
Member

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.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 21, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@@ -42,11 +54,35 @@ private static LessThan lessThanOf(Expression left, Expression right) {
return new LessThan(EMPTY, left, right, null);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

These mirror the functions of the same name in TestUtils, except these return the ESQL versions of the relevant expressions. In the future, it may be worth refactoring these into an EsqlTestUtils class, or maybe just inlining them as they're very small. For now, I think duplicating them like this is fine.

public void testEliminateRangeByEqualsOutsideInterval() {
FieldAttribute fa = getFieldAttribute();
Equals eq1 = equalsOf(fa, new Literal(EMPTY, 10, DataTypes.INTEGER));
Range r = rangeOf(fa, ONE, false, new Literal(EMPTY, 10, DataTypes.INTEGER), false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we don't have explicit ranges in ES|QL, these tests seemed worth keeping, in case optimization (current or future) rewrite other comparisons into Ranges.

@costin costin requested review from astefan and bpintea March 22, 2024 00:04
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @not-napoleon !

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

It's looking good Mark.
I think there are two more tests in OptimizerRulesTests that use this rule and can be moved, not necessarily because we have this kind of support (1<a<2) but for completeness sake. Besides, I won't be surprised if in the near future we add support for that sort of between binary operator.
The methods are testEliminateRangeByEqualsInInterval and testEliminateRangeByNullEqualsInInterval.

@not-napoleon
Copy link
Member Author

@astefan ah, cool. I'll migrate those tests over, then merge this. Thanks for the review, and also thank you @alex-spies !

@not-napoleon not-napoleon merged commit ecb4223 into elastic:main Mar 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants