ESQL: Refactor named writeable entries for expressions and plans (ela…

…stic#117029) Move writable declarations outside the core classes to avoid errors (such as subClass.getNamedWritable()) and centralize them in a top package class for better management. Make all touched serialization code the same as in the initial PR, except for the fact that LookupJoin/LookupJoinExec are missing. Co-authored-by: Costin Leau <[email protected]> (cherry picked from commit 0b74492)
costin · Nov 19, 2024 · 147974e · 147974e
1 parent c029282
commit 147974e
Show file tree

Hide file tree

Showing 41 changed files with 674 additions and 493 deletions.
diff --git a/...lugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java b/...lugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java
@@ -6,7 +6,6 @@
  */
 package org.elasticsearch.xpack.esql.core.expression;
 
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.xpack.esql.core.tree.Source;
 import org.elasticsearch.xpack.esql.core.type.DataType;
@@ -34,11 +33,6 @@ public abstract class Attribute extends NamedExpression {
      */
     protected static final String SYNTHETIC_ATTRIBUTE_NAME_PREFIX = "$$";
 
-    public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
-        // TODO add UnsupportedAttribute when these are moved to the same project
-        return List.of(FieldAttribute.ENTRY, MetadataAttribute.ENTRY, ReferenceAttribute.ENTRY);
-    }
-
     // can the attr be null - typically used in JOINs
     private final Nullability nullability;
 

diff --git a/...ugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java b/...ugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java
@@ -6,7 +6,6 @@
  */
 package org.elasticsearch.xpack.esql.core.expression;
 
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException;
 import org.elasticsearch.xpack.esql.core.capabilities.Resolvable;
 import org.elasticsearch.xpack.esql.core.capabilities.Resolvables;
@@ -15,7 +14,6 @@
 import org.elasticsearch.xpack.esql.core.type.DataType;
 import org.elasticsearch.xpack.esql.core.util.StringUtils;
 
-import java.util.ArrayList;
 import java.util.List;
 import java.util.function.Supplier;
 
@@ -29,14 +27,6 @@
  * (which is a type of expression) with a single child, c.
  */
 public abstract class Expression extends Node<Expression> implements Resolvable {
-    public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
-        List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
-        for (NamedWriteableRegistry.Entry e : NamedExpression.getNamedWriteables()) {
-            entries.add(new NamedWriteableRegistry.Entry(Expression.class, e.name, in -> (NamedExpression) e.reader.read(in)));
-        }
-        entries.add(Literal.ENTRY);
-        return entries;
-    }
 
     public static class TypeResolution {
         private final boolean failed;

diff --git a/...e/src/main/java/org/elasticsearch/xpack/esql/core/expression/ExpressionCoreWritables.java b/...e/src/main/java/org/elasticsearch/xpack/esql/core/expression/ExpressionCoreWritables.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.esql.core.expression;
+
+import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class ExpressionCoreWritables {
+
+    public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
+        List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
+        entries.addAll(expressions());
+        entries.addAll(namedExpressions());
+        entries.addAll(attributes());
+        return entries;
+    }
+
+    public static List<NamedWriteableRegistry.Entry> expressions() {
+        List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
+        // add entries as expressions
+        for (NamedWriteableRegistry.Entry e : namedExpressions()) {
+            entries.add(new NamedWriteableRegistry.Entry(Expression.class, e.name, in -> (Expression) e.reader.read(in)));
+        }
+        entries.add(Literal.ENTRY);
+        return entries;
+    }
+
+    public static List<NamedWriteableRegistry.Entry> namedExpressions() {
+        List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
+        // add entries as named writeables
+        for (NamedWriteableRegistry.Entry e : attributes()) {
+            entries.add(new NamedWriteableRegistry.Entry(NamedExpression.class, e.name, in -> (NamedExpression) e.reader.read(in)));
+        }
+        entries.add(Alias.ENTRY);
+        return entries;
+    }
+
+    public static List<NamedWriteableRegistry.Entry> attributes() {
+        return List.of(FieldAttribute.ENTRY, MetadataAttribute.ENTRY, ReferenceAttribute.ENTRY);
+    }
+}
diff --git a/...esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NamedExpression.java b/...esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NamedExpression.java
@@ -7,11 +7,9 @@
 package org.elasticsearch.xpack.esql.core.expression;
 
 import org.elasticsearch.common.io.stream.NamedWriteable;
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.xpack.esql.core.tree.Source;
 
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
 
@@ -20,14 +18,6 @@
  * (by converting to an attribute).
  */
 public abstract class NamedExpression extends Expression implements NamedWriteable {
-    public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
-        List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
-        for (NamedWriteableRegistry.Entry e : Attribute.getNamedWriteables()) {
-            entries.add(new NamedWriteableRegistry.Entry(NamedExpression.class, e.name, in -> (NamedExpression) e.reader.read(in)));
-        }
-        entries.add(Alias.ENTRY);
-        return entries;
-    }
 
     private final String name;
     private final NameId id;

diff --git a/...va/org/elasticsearch/xpack/esql/core/expression/predicate/fulltext/FullTextPredicate.java b/...va/org/elasticsearch/xpack/esql/core/expression/predicate/fulltext/FullTextPredicate.java
@@ -6,7 +6,6 @@
  */
 package org.elasticsearch.xpack.esql.core.expression.predicate.fulltext;
 
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.core.Nullable;
@@ -23,10 +22,6 @@
 
 public abstract class FullTextPredicate extends Expression {
 
-    public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
-        return List.of(MatchQueryPredicate.ENTRY, MultiMatchQueryPredicate.ENTRY, StringQueryPredicate.ENTRY);
-    }
-
     public enum Operator {
         AND,
         OR;

diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Block.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Block.java
@@ -10,16 +10,13 @@
 import org.apache.lucene.util.Accountable;
 import org.apache.lucene.util.RamUsageEstimator;
 import org.elasticsearch.common.io.stream.NamedWriteable;
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.core.RefCounted;
 import org.elasticsearch.core.Releasable;
 import org.elasticsearch.core.ReleasableIterator;
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.index.mapper.BlockLoader;
 
-import java.util.List;
-
 /**
  * A Block is a columnar representation of homogenous data. It has a position (row) count, and
  * various data retrieval methods for accessing the underlying data that is stored at a given
@@ -291,19 +288,6 @@ static Block[] buildAll(Block.Builder... builders) {
         }
     }
 
-    static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
-        return List.of(
-            IntBlock.ENTRY,
-            LongBlock.ENTRY,
-            FloatBlock.ENTRY,
-            DoubleBlock.ENTRY,
-            BytesRefBlock.ENTRY,
-            BooleanBlock.ENTRY,
-            ConstantNullBlock.ENTRY,
-            CompositeBlock.ENTRY
-        );
-    }
-
     /**
      * Serialization type for blocks: 0 and 1 replace false/true used in pre-8.14
      */

diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/BlockWritables.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/BlockWritables.java
@@ -0,0 +1,28 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.compute.data;
+
+import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
+
+import java.util.List;
+
+public class BlockWritables {
+
+    public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
+        return List.of(
+            IntBlock.ENTRY,
+            LongBlock.ENTRY,
+            FloatBlock.ENTRY,
+            DoubleBlock.ENTRY,
+            BytesRefBlock.ENTRY,
+            BooleanBlock.ENTRY,
+            ConstantNullBlock.ENTRY,
+            CompositeBlock.ENTRY
+        );
+    }
+}
diff --git a/...ugin/esql/compute/src/test/java/org/elasticsearch/compute/data/SerializationTestCase.java b/...ugin/esql/compute/src/test/java/org/elasticsearch/compute/data/SerializationTestCase.java
@@ -29,7 +29,7 @@
 public abstract class SerializationTestCase extends ESTestCase {
     BigArrays bigArrays;
     protected BlockFactory blockFactory;
-    NamedWriteableRegistry registry = new NamedWriteableRegistry(Block.getNamedWriteables());
+    NamedWriteableRegistry registry = new NamedWriteableRegistry(BlockWritables.getNamedWriteables());
 
     @Before
     public final void newBlockFactory() {

diff --git a/...mpute/src/test/java/org/elasticsearch/compute/operator/exchange/ExchangeServiceTests.java b/...mpute/src/test/java/org/elasticsearch/compute/operator/exchange/ExchangeServiceTests.java
@@ -22,8 +22,8 @@
 import org.elasticsearch.common.util.PageCacheRecycler;
 import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
-import org.elasticsearch.compute.data.Block;
 import org.elasticsearch.compute.data.BlockFactory;
+import org.elasticsearch.compute.data.BlockWritables;
 import org.elasticsearch.compute.data.IntBlock;
 import org.elasticsearch.compute.data.MockBlockFactory;
 import org.elasticsearch.compute.data.Page;
@@ -457,7 +457,7 @@ public void sendResponse(TransportResponse transportResponse) {
 
     private MockTransportService newTransportService() {
         List<NamedWriteableRegistry.Entry> namedWriteables = new ArrayList<>(ClusterModule.getNamedWriteables());
-        namedWriteables.addAll(Block.getNamedWriteables());
+        namedWriteables.addAll(BlockWritables.getNamedWriteables());
         NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(namedWriteables);
         MockTransportService service = MockTransportService.createNewService(
             Settings.EMPTY,