More code review fixes

Author: GalLalouche

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestUtils.java

@@ -120,9 +120,7 @@ public static Tuple<Version, Version> skipVersionRange(String testName, String i
         return null;
     }
 
-    public record PageColumn(String name, DataType dataType) {}
-
-    public static Tuple<Page, List<PageColumn>> loadPageFromCsv(URL source, Map<String, String> typeMapping) throws Exception {
+    public static Tuple<Page, List<String>> loadPageFromCsv(URL source, Map<String, String> typeMapping) throws Exception {
 
         record CsvColumn(String name, Type type, BuilderWrapper builderWrapper) implements Releasable {
             void append(String stringValue) {
@@ -232,10 +230,10 @@ public void close() {
                 lineNumber++;
             }
         }
-        var pageColumns = new ArrayList<PageColumn>(columns.length);
+        var pageColumns = new ArrayList<String>(columns.length);
         try {
             var blocks = Arrays.stream(columns)
-                .peek(b -> pageColumns.add(new PageColumn(b.name, DataType.fromTypeName(b.type.name()))))
+                .peek(b -> pageColumns.add(b.name))
                 .map(b -> b.builderWrapper.builder().build())
                 .toArray(Block[]::new);
             return new Tuple<>(new Page(blocks), pageColumns);

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

@@ -669,13 +669,13 @@ private List<Attribute> resolveUsingColumns(List<Attribute> cols, List<Attribute
             List<Attribute> resolved = new ArrayList<>(cols.size());
             for (Attribute col : cols) {
                 if (col instanceof UnresolvedAttribute ua) {
-                    Attribute resolvedCol = maybeResolveAttribute(ua, output);
-                    if (resolvedCol instanceof UnresolvedAttribute ucol) {
+                    Attribute resolvedField = maybeResolveAttribute(ua, output);
+                    if (resolvedField instanceof UnresolvedAttribute ucol) {
                         String message = ua.unresolvedMessage();
                         String match = "column [" + ucol.name() + "]";
-                        resolvedCol = ucol.withUnresolvedMessage(message.replace(match, match + " in " + side + " side of join"));
+                        resolvedField = ucol.withUnresolvedMessage(message.replace(match, match + " in " + side + " side of join"));
                     }
-                    resolved.add(resolvedCol);
+                    resolved.add(resolvedField);
                 } else {
                     throw new IllegalStateException(
                         "Surprised to discover column [ " + col.name() + "] already resolved when resolving JOIN keys"
@@ -686,16 +686,16 @@ private List<Attribute> resolveUsingColumns(List<Attribute> cols, List<Attribute
         }
 
         private LogicalPlan resolveInsist(Insist insist, List<Attribute> childrenOutput, IndexResolution indexResolution) {
-            Attribute resolvedCol = maybeResolveAttribute(insist.getInsistIdentifier(), childrenOutput);
+            Attribute resolvedCol = maybeResolveAttribute(insist.attribute(), childrenOutput);
             // Field isn't mapped anywhere.
             if (resolvedCol instanceof UnresolvedAttribute) {
-                return pushdownInsist(insist, attrs -> attrs.add(insistKeyword(insist)));
+                return mergeInsist(insist, attrs -> attrs.add(insistKeyword(insist)));
             }
 
             String name = resolvedCol.name();
             // Field is partially unmapped.
-            if (resolvedCol instanceof FieldAttribute f && indexResolution.get().partiallyUnmappedFields().contains(name)) {
-                return pushdownInsist(insist, attrs -> {
+            if (resolvedCol instanceof FieldAttribute f && indexResolution.get().isPartiallyUnmappedField(name)) {
+                return mergeInsist(insist, attrs -> {
                     var index = CollectionUtils.findFirstIndex(attrs, e -> e.name().equals(name)).getAsInt();
                     Attribute attribute = f.field().getDataType() == KEYWORD ? insistKeyword(insist) : invalidInsistAttribute(insist, f);
                     attrs.set(index, attribute);
@@ -706,7 +706,8 @@ private LogicalPlan resolveInsist(Insist insist, List<Attribute> childrenOutput,
             return insist.child();
         }
 
-        private static EsRelation pushdownInsist(Insist insist, Consumer<List<Attribute>> updateAttributes) {
+        private static EsRelation mergeInsist(Insist insist, Consumer<List<Attribute>> updateAttributes) {
+            assert insist.child() instanceof EsRelation : "INSIST should be on top of a relation (see LogicalPlanBuilder)";
             var relation = (EsRelation) insist.child();
             List<Attribute> newOutput = new ArrayList<>(relation.output());
             updateAttributes.accept(newOutput);
@@ -722,7 +723,7 @@ private static UnsupportedAttribute invalidInsistAttribute(Insist insist, FieldA
         }
 
         private static FieldAttribute insistKeyword(Insist insist) {
-            String name = insist.getInsistIdentifier().name();
+            String name = insist.attribute().name();
             return new FieldAttribute(insist.source(), name, new PotentiallyUnmappedKeywordEsField(name));
         }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/index/EsIndex.java

@@ -13,6 +13,7 @@
 import org.elasticsearch.index.IndexMode;
 import org.elasticsearch.xpack.esql.core.type.EsField;
 
+import java.beans.Transient;
 import java.io.IOException;
 import java.util.Map;
 import java.util.Set;
@@ -24,7 +25,7 @@ public record EsIndex(
     Map<String, EsField> mapping,
     Map<String, IndexMode> indexNameWithModes,
     /** Fields mapped only in some (but *not* all) indices. Since this is only used by the analyzer, it is not serialized. */
-    Set<String> partiallyUnmappedFields
+    @Transient Set<String> partiallyUnmappedFields
 ) implements Writeable {
 
     public EsIndex {
@@ -81,6 +82,10 @@ public void writeTo(StreamOutput out) throws IOException {
         // partially unmapped fields shouldn't pass the coordinator node anyway, since they are only used by the Analyzer.
     }
 
+    public boolean isPartiallyUnmappedField(String fieldName) {
+        return partiallyUnmappedFields.contains(fieldName);
+    }
+
     public Set<String> concreteIndices() {
         return indexNameWithModes.keySet();
     }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Insist.java

@@ -19,11 +19,11 @@
 import java.util.Objects;
 
 public class Insist extends UnaryPlan {
-    private final UnresolvedAttribute insistIdentifier;
+    private final UnresolvedAttribute insistedAttribute;
 
-    public Insist(Source source, UnresolvedAttribute insistIdentifier, LogicalPlan child) {
+    public Insist(Source source, UnresolvedAttribute insistedAttribute, LogicalPlan child) {
         super(source, child);
-        this.insistIdentifier = insistIdentifier;
+        this.insistedAttribute = insistedAttribute;
     }
 
     private @Nullable List<Attribute> lazyOutput = null;
@@ -38,27 +38,27 @@ public List<Attribute> output() {
 
     private List<Attribute> computeOutput() {
         var result = new ArrayList<>(child().output());
-        result.add(insistIdentifier);
+        result.add(insistedAttribute);
         return result;
     }
 
-    public UnresolvedAttribute getInsistIdentifier() {
-        return insistIdentifier;
+    public UnresolvedAttribute attribute() {
+        return insistedAttribute;
     }
 
     @Override
     public Insist replaceChild(LogicalPlan newChild) {
-        return new Insist(source(), insistIdentifier, newChild);
+        return new Insist(source(), insistedAttribute, newChild);
     }
 
     @Override
     public boolean expressionsResolved() {
-        return computeOutput().stream().allMatch(Attribute::resolved);
+        return insistedAttribute.resolved();
     }
 
     @Override
     protected NodeInfo<? extends LogicalPlan> info() {
-        return NodeInfo.create(this, Insist::new, insistIdentifier, child());
+        return NodeInfo.create(this, Insist::new, insistedAttribute, child());
     }
 
     @Override
@@ -73,11 +73,11 @@ public String getWriteableName() {
 
     @Override
     public int hashCode() {
-        return Objects.hash(super.hashCode(), Objects.hashCode(insistIdentifier));
+        return Objects.hash(super.hashCode(), Objects.hashCode(insistedAttribute));
     }
 
     @Override
     public boolean equals(Object obj) {
-        return super.equals(obj) && ((Insist) obj).insistIdentifier.equals(insistIdentifier);
+        return super.equals(obj) && ((Insist) obj).insistedAttribute.equals(insistedAttribute);
     }
 }