ESQL: Generate evaluator factories #100516

nik9000 · 2023-10-09T14:06:05Z

This generates the evaluator factories which we'd been implementing inline with ->. The advantage of generating them is that they have a useful toString and class name. These are useful for debugging and let us remove a kind of errant use ThrowingDriverContext just to generate a factory description for logging. Now we can just use the generated toString.

This generates the evaluator factories which we'd been implementing inline with `->`. The advantage of generating them is that they have a useful `toString` and class name. These are useful for debugging and let us remove a kind of errant use `ThrowingDriverContext` just to generate a factory description for logging. Now we can just use the generated `toString`.

elasticsearchmachine · 2023-10-09T14:06:31Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-10-09T14:06:31Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

nik9000 · 2023-10-09T14:08:02Z

This is almost 100% mechanical. I sort of did it out of anger to remove a weird toString. But I do think it's nicer.

nik9000 · 2023-10-09T17:13:20Z

run elasticsearch-ci/part-1

bpintea

I first thought that the change is rather large just for factory's toString(), but then not having to inline these factories seems quite comfy too, so it LGTM.

bpintea · 2023-10-25T11:46:47Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/MultivalueDedupe.java

@@ -156,11 +143,40 @@ public static BatchEncoder batchEncoder(Block.Ref ref, int batchSize, boolean al
        }
    }

-    private abstract static class MvDedupeEvaluator implements EvalOperator.ExpressionEvaluator {
-        protected final EvalOperator.ExpressionEvaluator field;
+    private static class EvaluatorFactory implements ExpressionEvaluator.Factory {


Could this and the Evaluator below not be a record, to make these more concise?
(And same for the evaluator implementers.)

I can do that here. The other ones I'm kind of stuck on because javapoet can't make records. Or, at least, it couldn't when I last checked.

square/javapoet#981 for reference.

bpintea · 2023-10-25T11:47:23Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/MultivalueDedupe.java

+
+        @Override
+        public String toString() {
+            return "MvDedupe[field=" + field + "]";


Wouldn't we want to distinguish the factory from the produced object itself? I.e. should this be a "MvDedupeFactory[..."?

Previously we were didn't use that in the toString and I think that's pretty ok here because it end up some of the debug information.

bpintea · 2023-10-25T12:02:21Z

...a/org/elasticsearch/xpack/esql/evaluator/predicate/operator/comparison/ComparisonMapper.java

-        return dvrCtx -> buildEvaluator.apply(lhs.get(dvrCtx), rhs.get(dvrCtx), dvrCtx);
-    }
-
-    public static ExpressionEvaluator.Factory castToEvaluatorWithSource(


alex-spies

Didn't look at all files since most changes are mechanical.
LGTM except that I find the EvaluatorImplementer a bit confusing to read since we emit code for either the factory or the outer class based on a switch that's applied to nearly every function - IMHO separating into two sets of functions would be easier to follow since the code already comes with a lot of indirections (since it generates code).

x-pack/plugin/esql/compute/ann/src/main/java/org/elasticsearch/compute/ann/Fixed.java

alex-spies · 2023-10-25T16:12:34Z

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

@@ -99,15 +104,22 @@ private TypeSpec type() {
        return builder.build();
    }

-    private MethodSpec ctor() {
+    private MethodSpec ctor(Implementing implementing) {


suggestion: I found this confusing to read; I think it would be simpler to just have a private MethodSpec ctor() and a private MethodSpec factoryCtor().

The code duplication is probably negligible - and I think it's fair to have 2 different methods, given that we also emit 2 methods that are not inherently related.

alex-spies · 2023-10-25T16:17:46Z

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

@@ -439,16 +512,28 @@ public String paramName(boolean blockStyle) {
        }

        @Override
-        public void declareField(TypeSpec.Builder builder) {
-            builder.addField(ArrayTypeName.of(EXPRESSION_EVALUATOR), name, Modifier.PRIVATE, Modifier.FINAL);
+        public void declareField(Implementing implementing, TypeSpec.Builder builder) {


Similarly here, I think we might as well replace the implementing argument by an array type name - this would make code that call declareField easier to understand.

Maybe it'd be better to get rid of the Implementing enum.

nik9000 · 2023-10-25T16:52:47Z

I first thought that the change is rather large just for factory's toString(), but then not having to inline these factories seems quite comfy too, so it LGTM.

It really is too big! It's kind of a sign that generating all this code ain't great. Or, at least, committing the generated code.....

…/compute/ann/Fixed.java Co-authored-by: Alexander Spies <[email protected]>

…think' into esql_gen_tostring_think

nik9000 added >non-issue :Analytics/ES|QL AKA ESQL v8.12.0 labels Oct 9, 2023

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 9, 2023

nik9000 added 3 commits October 9, 2023 11:30

Fix test

7432b14

No really

2bc3281

Format

8dc6fbe

nik9000 mentioned this pull request Oct 10, 2023

Block and Vector creation can start to use a block factory that performs circuit breaking - 8.12 #100528

Closed

21 tasks

bpintea approved these changes Oct 25, 2023

View reviewed changes

alex-spies approved these changes Oct 25, 2023

View reviewed changes

nik9000 and others added 5 commits October 25, 2023 12:57

Update x-pack/plugin/esql/compute/ann/src/main/java/org/elasticsearch…

2717216

…/compute/ann/Fixed.java Co-authored-by: Alexander Spies <[email protected]>

Merge branch 'main' into esql_gen_tostring_think

78d0f8b

Merge remote-tracking branch 'refs/remotes/nik9000/esql_gen_tostring_…

3263c33

…think' into esql_gen_tostring_think

Simplify builder

437fc08

Make it a record

0615cc8

nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 25, 2023

elasticsearchmachine merged commit 9796de4 into elastic:main Oct 25, 2023
12 checks passed

nik9000 deleted the esql_gen_tostring_think branch October 25, 2023 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Generate evaluator factories #100516

ESQL: Generate evaluator factories #100516

nik9000 commented Oct 9, 2023

elasticsearchmachine commented Oct 9, 2023

elasticsearchmachine commented Oct 9, 2023

nik9000 commented Oct 9, 2023

nik9000 commented Oct 9, 2023

bpintea left a comment •

edited

Loading

bpintea Oct 25, 2023

nik9000 Oct 25, 2023

nik9000 Oct 25, 2023

bpintea Oct 25, 2023

nik9000 Oct 25, 2023

bpintea Oct 25, 2023

alex-spies left a comment

alex-spies Oct 25, 2023

nik9000 Oct 25, 2023

alex-spies Oct 25, 2023

nik9000 commented Oct 25, 2023

ESQL: Generate evaluator factories #100516

ESQL: Generate evaluator factories #100516

Conversation

nik9000 commented Oct 9, 2023

elasticsearchmachine commented Oct 9, 2023

elasticsearchmachine commented Oct 9, 2023

nik9000 commented Oct 9, 2023

nik9000 commented Oct 9, 2023

bpintea left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 25, 2023

bpintea left a comment •

edited

Loading