[ES|QL] Rewrite In with EvaluatorMapper by fang-xing-esql · Pull Request #109413 · elastic/elasticsearch

fang-xing-esql · 2024-06-06T02:04:02Z

This is a subtask of #106679 to replace EvalMapper with EvaluatorMapper. There are multiple functions/predicates that use EvalMapper, this PR focus on the IN predicate.

The result of In predicate can be different whether the inlist has nulls in it or not. Multi-valued fields/expressions are not support by In predicate.

The results below are collected from postgres, these are used as the expected results.
test=# select * from test;
 c1 | c2
----+----
  1 | a
  2 | b
  3 | c
  4 | d
  5 |
(5 rows)

When there is null in the inlist, the return value of in can be either true or null, depending whether there is a matching between LHS and RHS. If LHS is null, in returns null no matter the inlist has null or not.
test=# select c1, c2, c2 in ('e', 'f') as c3 from test;
 c1 | c2 | c3
----+----+----
  1 | a  | f
  2 | b  | f
  3 | c  | f
  4 | d  | f
  5 |    |
(5 rows)

test=# select c1, c2, c2 in ('a', 'f', null) as c3 from test;
 c1 | c2 | c3
----+----+----
  1 | a  | t
  2 | b  |
  3 | c  |
  4 | d  |
  5 |    |
(5 rows)

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

elasticsearchmachine · 2024-06-06T14:53:08Z

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

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

x-pack/plugin/esql/qa/testFixtures/src/main/resources/ip.csv-spec

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java

nik9000

I like this much more! I left a few questions, but I think this seems like the right thing to do for IN.

...rg/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InBooleanEvaluator.java

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java

nik9000 · 2024-07-10T16:42:26Z

...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlExpressionTranslators.java

        }
    }
+
+    public static class InComparisons extends ExpressionTranslator<In> {


I think we're moving these into their own top level file more often these days.

I think we're moving these into their own top level file more often these days.

I just double checked, a lot of LogicalPlanOptimizer rules are in their own top level files, under optimizer/rules, the ExpressionTranslators are not in their own files yet. They are in either EsqlExpressionTranslators or ExpressionTranslators in esql-core. Perhaps keep it here for now?

Either is fine by me.

nik9000 · 2024-07-10T16:48:08Z

...rg/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InBooleanEvaluator.java

+                    continue;
+                }
+                try {
+                    In.process(result, nulls, mvs, lhsBlock.getBoolean(lhsBlock.getFirstValueIndex(p)), rhsValues);


I wonder - should booleans be special and not use the array method? Ultimately IN (true) and IN (false) and IN () and IN (false, true) are the only possible layouts. Right? IN (false, false) has the same meaning as IN (false)`. I think.

If that's true, then this is useful: I've found sometimes I don't use the string template stuff for boolean because it's so different. In this case maybe I'd slurp more of this method into In.process's boolean flavor. But whatever make the hacking easy I'm for. I don't have a strong opinion about the right way to lay out these templates.

Indeed, I'll simplify boolean. I ended up modifying the template to generate evaluator for boolean, as it makes it easier to handle warnings consistently with other types.

...g/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InBytesRefEvaluator.java

fang-xing-esql · 2024-07-11T14:55:14Z

I like this much more! I left a few questions, but I think this seems like the right thing to do for IN.

Thanks for reviewing it again @nik9000 ! Most of the comments are addressed, and I cleaned it up according to the suggestions, simplified boolean a bit, left InComparisons in EsqlExpressionTranslators for now as the other ExpressionTranslators.

nik9000

Looks good to me. I left a question about the Analyzer but that's pretty far outside my area of expertise so I'm happy to trust about moving that invocation rather than merging.

nik9000 · 2024-07-12T15:41:26Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

            }
-            if (f instanceof In in) {
-                return processIn(in);
-            }


Can we use processScalarFunction for this one or is it incompatible somehow?

Can we use processScalarFunction for this one or is it incompatible somehow?

This is a good point! It is not quite compatible for now, one reason is that In is not a registered function yet, it is under operator/comparison, there will be a little more work to make it a registered function. I'll look into it, and if possible, create a follow up PR for it.

nik9000 · 2024-07-12T15:42:27Z

...va/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InEvaluator.java.st

+import org.elasticsearch.compute.data.BytesRefVector;
+$elseif(boolean)$
+import org.elasticsearch.compute.data.BooleanVector;
+$endif$


I've tried to remove the import order rules from this generated code but it's a huge pain. I'm sorry about this mess.

nik9000 · 2024-07-12T15:43:01Z

...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlExpressionTranslators.java

        }
    }
+
+    public static class InComparisons extends ExpressionTranslator<In> {


Either is fine by me.

Quick cleanup PR. With #109413 merged, nothing in ESQL production depends on the core binary comparison classes anymore. This PR removes them to avoid the potential for confusion, and open the opportunity for merging the core `BinaryComparison` class with the `EsqlBinaryComparison` base class, which is now its only direct inheritor (work to be done in a future PR).

* rewrite in with EvaluatorMapper

Quick cleanup PR. With #109413 merged, nothing in ESQL production depends on the core binary comparison classes anymore. This PR removes them to avoid the potential for confusion, and open the opportunity for merging the core `BinaryComparison` class with the `EsqlBinaryComparison` base class, which is now its only direct inheritor (work to be done in a future PR).

* rewrite in with EvaluatorMapper

Quick cleanup PR. With #109413 merged, nothing in ESQL production depends on the core binary comparison classes anymore. This PR removes them to avoid the potential for confusion, and open the opportunity for merging the core `BinaryComparison` class with the `EsqlBinaryComparison` base class, which is now its only direct inheritor (work to be done in a future PR).

fang-xing-esql added 7 commits June 3, 2024 21:00

rewrite in with EvaluatorMapper

fa3de4b

rewrite inlist

0b2b4ce

Merge branch 'main' into inlist

22ec386

rewrite inlist

aa11319

Merge branch 'main' into inlist

6361252

evaluator

23e43bb

EsqlCapabilities

8196f9d

fang-xing-esql added >refactoring :Analytics/ES|QL AKA ESQL v8.15.0 labels Jun 6, 2024

fang-xing-esql commented Jun 6, 2024

View reviewed changes

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java Outdated Show resolved Hide resolved

Merge branch 'main' into inlist

1c24b8b

fang-xing-esql marked this pull request as ready for review June 6, 2024 14:52

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 6, 2024

nik9000 reviewed Jun 6, 2024

View reviewed changes

not-napoleon reviewed Jun 6, 2024

View reviewed changes

x-pack/plugin/esql/qa/testFixtures/src/main/resources/ip.csv-spec Outdated Show resolved Hide resolved

not-napoleon reviewed Jun 6, 2024

View reviewed changes

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java Show resolved Hide resolved

fang-xing-esql added 5 commits June 6, 2024 13:26

ExpressionTranslator for esql in

bfd17fb

update evaluator

7e531a4

Merge branch 'main' into inlist

706947b

add BlockArrayProcessFunctionArg to EvaluatorImplementer

f7c08c5

Merge branch 'main' into inlist

fb8776a

not-napoleon reviewed Jun 10, 2024

View reviewed changes

not-napoleon mentioned this pull request Jun 10, 2024

[ESQL] Remove Core BooleanComparison classes #109553

Closed

fang-xing-esql added 3 commits June 11, 2024 10:35

Merge branch 'main' into inlist

5c1c6cb

Merge branch 'main' into inlist

3de7171

added some comments for ProcessFunctionArg

06bfa53

nik9000 reviewed Jun 12, 2024

View reviewed changes

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java Outdated Show resolved Hide resolved

Merge branch 'main' into inlist

ae2c5d1

fang-xing-esql requested a review from a team as a code owner June 17, 2024 18:55

mark-vieira approved these changes Jun 17, 2024

View reviewed changes

fang-xing-esql added 2 commits June 17, 2024 15:22

spotless

8729f18

Merge branch 'main' into inlist

5d26389

elasticsearchmachine added v8.16.0 and removed v8.15.0 labels Jul 4, 2024

fang-xing-esql added 5 commits July 8, 2024 13:50

Merge branch 'main' into inlist

31760dc

Merge branch 'main' into inlist

23d6f76

pick up changes in main

8a6097e

clean up

8f20e6b

Merge branch 'main' into inlist

b215c0c

nik9000 reviewed Jul 10, 2024

View reviewed changes

fang-xing-esql added 5 commits July 10, 2024 13:02

Merge branch 'main' into inlist

7749f55

update according to review comments

270e756

cleanup

bb2fbb0

Merge branch 'main' into inlist

1a2db8d

cleanup

3f0769e

Merge branch 'main' into inlist

5e8fed7

fang-xing-esql mentioned this pull request Jul 11, 2024

[ES|QL] Pushdown string format for BinaryComparison on datetime fields #110669

Merged

Merge branch 'main' into inlist

26c9a80

nik9000 approved these changes Jul 12, 2024

View reviewed changes

fang-xing-esql merged commit 0be34fe into elastic:main Jul 12, 2024

fang-xing-esql deleted the inlist branch July 12, 2024 18:15

not-napoleon mentioned this pull request Jul 15, 2024

[ESQL} Remove unused core binary comparison classes #110891

Merged

tvernum pushed a commit that referenced this pull request Feb 25, 2025

[ES|QL] Rewrite In with EvaluatorMapper (#109413)

09cc2dc

* rewrite in with EvaluatorMapper

tvernum pushed a commit that referenced this pull request Feb 25, 2025

[ES|QL] Rewrite In with EvaluatorMapper (#109413)

088a187

* rewrite in with EvaluatorMapper

Conversation

fang-xing-esql commented Jun 6, 2024 • edited by nik9000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fang-xing-esql commented Jul 11, 2024

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fang-xing-esql commented Jun 6, 2024 •

edited by nik9000

Loading

fang-xing-esql Jul 10, 2024 •

edited

Loading