Address feedback

Author: costin

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

@@ -648,6 +648,7 @@ private Join resolveLookupJoin(LookupJoin join) {
  List<Attribute> leftKeys = resolveUsingColumns(cols, join.left().output(), "left");
  List<Attribute> rightKeys = resolveUsingColumns(cols, join.right().output(), "right");
  List<Attribute> output = new ArrayList<>(join.left().output());
+ // the order is stable (since the AttributeSet preservers the insertion order)
  output.addAll(join.right().outputSet().subtract(new AttributeSet(rightKeys)));
 
  // update the config - pick the left keys as those in the output

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

@@ -23,8 +23,6 @@
 
 /**
  * Lookup join - specialized LEFT (OUTER) JOIN between the main left side and a lookup index (index_mode = lookup) on the right.
- * In the future, as the join capabilities of the engine will evolve, a regular LEFT JOIN will be used instead, letting the planner decide
- * the strategy at runtime.
  */
 public class LookupJoin extends Join implements SurrogateLogicalPlan {
 
@@ -62,8 +60,6 @@ public Join replaceChildren(LogicalPlan left, LogicalPlan right) {
 
  @Override
  protected NodeInfo<Join> info() {
- // Do not just add the JoinConfig as a whole - this would prevent correctly registering the
- // expressions and references.
  return NodeInfo.create(this, LookupJoin::new, left(), right(), config(), output);
  }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java

@@ -116,12 +116,14 @@ public List<Attribute> output() {
 
  @Override
  public AttributeSet inputSet() {
- // TODO: this is a hack until qualifiers land since the right side is always materialized
+ // TODO: this is a hack since the right side is always materialized - instead this should
+ // return the _doc so the extraction can happen lazily
  return left().outputSet();
  }
 
  @Override
  protected AttributeSet computeReferences() {
+ // TODO: same as above - once lazy materialization of both sides lands, this needs updating
  return Expressions.references(leftFields);
  }
 

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

@@ -1945,18 +1945,14 @@ public void testLookup() {
  .item(startsWith("job{f}"))
  .item(startsWith("job.raw{f}"))
  /*
- * Int is a reference here because we renamed it in project.
- * If we hadn't it'd be a field and that'd be fine.
+ * Int key is returned as a full field (despite the rename)
  */
  .item(containsString("int{f}"))
  .item(startsWith("last_name{f}"))
  .item(startsWith("long_noidx{f}"))
  .item(startsWith("salary{f}"))
  /*
- * It's important that name is returned as a *reference* here
- * instead of a field. If it were a field we'd use SearchStats
- * on it and discover that it doesn't exist in the index. It doesn't!
- * We don't expect it to. It exists only in the lookup table.
+ * As is the name column from the right side.
  */
  .item(containsString("name{f}"))
  );