[ES|QL] Add CombineBinaryComparisons rule

[fang-xing-esql]

Resolves #108525

This rule exists in sql, but not esql. After adding this rule the OR predicate in the test can be transformed into a boolean term. The sql rule is modified as ES|QL doesn't support range yet.

[elasticsearchmachine]

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

[elasticsearchmachine]

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

[astefan]

I don't believe this change is enough.
This rule you are adding (from SQL) had tests in OptimizerRulesTests. We must add as many as possible back in ESQL.

[astefan]

Thinking about this some more, I think we didn't add this rule because of multi-values fields. EQL doesn't use this rule and EQL is multi-values aware. SQL, on the other hand, is single-value enforcer and uses it.

Please, look into the multi-values issue and see if this rule makes sense in this scenario.

[fang-xing-esql]

Thinking about this some more, I think we didn't add this rule because of multi-values fields. EQL doesn't use this rule and EQL is multi-values aware. SQL, on the other hand, is single-value enforcer and uses it.

Please, look into the multi-values issue and see if this rule makes sense in this scenario.

I'll look into multi-valued fields and migrating more tests, there must be a reason that this rule was not added for ES|QL in the first place.

[fang-xing-esql]

I don't believe this change is enough. This rule you are adding (from SQL) had tests in OptimizerRulesTests. We must add as many as possible back in ESQL.

Thanks for reviewing @astefan ! More tests(from OptimizerRulesTests) related to the CombineBinaryComparisons rule are added into esql in CombineBinaryComparisonsTests. The tests related to Range are not added, as Range is not supported in esql yet. The BinaryComparison operators the rule affects are ==, !=, >, >=, <, <=, they work with single value fields, they throw warnings if the fields are multi-valued. I did some tests on multi-valued fields, they seem work as expected.

[astefan]

It seems to be ok with multi-value fields, indeed. Thank you for checking.
I've left two more comments related to a better testing coverage.

[astefan]

LGTM. Thanks
Please, wait also for @bpintea's approval before merging.

[bpintea]

Why discard the Range formation? We're not having language support for smth. like BETWEEN, but I think Ranges could very well be used internally.

Right now an ESQL query like from employees | where salary > 30000 and salary < 50000
is translated into a DSL bool query with two distinctive range queries:

query[{"bool":{"must":[{"esql_single_value":{"field":"salary","next":{"range"
:{"salary":{"gt":30000,"boost":1.0}}},"source":"salary > 30000@5:38"}},{"esql_single_value":{"field":"salary","next":{"range":{"salary":{"lt":50000,"boost":1.0}}},"source":"salary  < 50000@5:57"}}],"boost":1.
0}}

We can optimise the filter output already and potentially enable other Range-based optimisation rules.

[fang-xing-esql]

Why discard the Range formation? We're not having language support for smth. like BETWEEN, but I think Ranges could very well be used internally.

Right now an ESQL query like from employees | where salary > 30000 and salary < 50000 is translated into a DSL bool query with two distinctive range queries:

query[{"bool":{"must":[{"esql_single_value":{"field":"salary","next":{"range"
:{"salary":{"gt":30000,"boost":1.0}}},"source":"salary > 30000@5:38"}},{"esql_single_value":{"field":"salary","next":{"range":{"salary":{"lt":50000,"boost":1.0}}},"source":"salary  < 50000@5:57"}}],"boost":1.
0}}

We can optimise the filter output already and potentially enable other Range-based optimisation rules.

That's a good point! Although BETWEEN is not supported in ES|QL yet, Range can be useful for pushing down predicates in physical plan, I'll look more into it.

[fang-xing-esql]

Thank you for reviewing @bpintea! As suggested, I created a separate PR #111255 to add both CombineBinaryComparisons and Range, it is a more complete solution, more complicated also :). I don't have a strong opinion to either including or not including Range, if we are going to support BETWEEN in ES|QL in the future, we will need Range at some point.

[elasticsearchmachine]

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

[fang-xing-esql]

Thank you for reviewing @bpintea @astefan ! It was a long discussion, but hopefully it worths it :-). I'll merge this once CI is clear.