Address more code review comments

julian-elastic · julian-elastic · commit 242455fd95ec · 2025-08-28T20:19:25.000-04:00
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java
@@ -387,7 +387,7 @@ public Query regexpQuery(
     }
 
     /**
-     * Returns a Lucine pushable Query for the current field
+     * Returns a Lucene pushable Query for the current field
      * For now can only be AutomatonQuery or MatchAllDocsQuery() or MatchNoDocsQuery()
      */
     public Query automatonQuery(
diff --git a/test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java b/test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java
@@ -774,7 +774,7 @@ private Map<String, Object> lookupExplosion(int sensorDataCount, int lookupEntri
             }
             if (lookupEntries != lookupEntriesToKeep) {
                 // add a filter to reduce the number of matches
-                // we add both a Lucine pushable filter and a non-pushable filter
+                // we add both a Lucene pushable filter and a non-pushable filter
                 // this is to make sure that even if there are non-pushable filters the pushable filters is still applied
                 query.append(" | WHERE ABS(filter_key) > -1 AND filter_key < ").append(lookupEntriesToKeep);
 
diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/EnrichQuerySourceOperator.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/EnrichQuerySourceOperator.java
@@ -159,7 +159,7 @@ Page buildPage(int positions, IntVector.Builder positionsBuilder, IntVector.Buil
         return page;
     }
 
-    private Query nextQuery() throws IOException {
+    private Query nextQuery() {
         ++queryPosition;
         while (isFinished() == false) {
             Query query = queryList.getQuery(queryPosition);
diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec
@@ -5228,6 +5228,69 @@ id_int:integer | name_str:keyword | extra1:keyword | other1:keyword | other2:int
 6              | null             | corge          | iota           | 9000
 ;
 
+lookupJoinWithCoalesceFilterOnRight
+required_capability: join_lookup_v12
+required_capability: lookup_join_on_multiple_fields
+
+FROM multi_column_joinable
+| LOOKUP JOIN multi_column_joinable_lookup ON id_int, is_active_bool
+| WHERE COALESCE(other1, "zeta") == "zeta"
+| KEEP id_int, name_str, extra1, other1, other2
+| SORT id_int, name_str, extra1, other1, other2
+| LIMIT 20
+;
+
+warning:Line 2:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON id_int, is_active_bool] failed, treating result as null. Only first 20 failures recorded.
+warning:Line 2:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
+
+id_int:integer | name_str:keyword | extra1:keyword | other1:keyword | other2:integer
+[1, 19, 21]    | null             | zyx            | null           | null
+4              | David            | qux            | zeta           | 6000
+9              | null             | waldo          | null           | null
+10             | null             | fred           | null           | null
+15             | null             | bar2           | null           | null
+[17, 18]       | null             | xyz            | null           | null
+null           | null             | plugh          | null           | null
+;
+
+lookupJoinWithIsNullFilterOnRight
+required_capability: join_lookup_v12
+required_capability: lookup_join_on_multiple_fields
+
+FROM multi_column_joinable
+| LOOKUP JOIN multi_column_joinable_lookup ON id_int, is_active_bool
+| WHERE other1 IS NULL
+| KEEP id_int, name_str, extra1, other1, other2
+| SORT id_int
+;
+
+warning:Line 2:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON id_int, is_active_bool] failed, treating result as null. Only first 20 failures recorded.
+warning:Line 2:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
+
+id_int:integer | name_str:keyword | extra1:keyword | other1:keyword | other2:integer
+[1, 19, 21]    | null             | zyx            | null           | null
+9              | null             | waldo          | null           | null
+10             | null             | fred           | null           | null
+15             | null             | bar2           | null           | null
+[17, 18]       | null             | xyz            | null           | null
+null           | null             | plugh          | null           | null
+;
+
+lookupJoinWithIsNullJoinKey
+required_capability: join_lookup_v12
+required_capability: lookup_join_on_multiple_fields
+
+FROM multi_column_joinable
+| LOOKUP JOIN multi_column_joinable_lookup ON name_str
+| WHERE name_str IS NULL
+| KEEP id_int, name_str, extra1, other1, other2
+| SORT id_int
+;
+
+id_int:integer | name_str:keyword | extra1:keyword | other1:keyword | other2:integer
+null           | null             | corge          | null           | null
+;
+
 lookupJoinWithMixLeftAndRightFilters
 required_capability: join_lookup_v12
 required_capability: lookup_join_on_multiple_fields
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
@@ -37,7 +37,7 @@
  * A {@link LookupEnrichQueryGenerator} that combines one or more {@link QueryList}s into a single query.
  * Each query in the resulting query will be a conjunction of all queries from the input lists at the same position.
  * In addition, we support an optional pre-join filter that will be applied to all queries if it is pushable.
- * If the pre-join filter cannot be pushed down to Lucine, it will be ignored.
+ * If the pre-join filter cannot be pushed down to Lucene, it will be ignored.
  */
 public class ExpressionQueryList implements LookupEnrichQueryGenerator {
     private static final Logger logger = LogManager.getLogger(ExpressionQueryList.class);
@@ -56,7 +56,7 @@ public ExpressionQueryList(
         }
         this.queryLists = queryLists;
         this.context = context;
-        buildPrePostJoinFilter(rightPreJoinPlan, clusterService);
+        buildPreJoinFilter(rightPreJoinPlan, clusterService);
     }
 
     private void addToPreJoinFilters(org.elasticsearch.index.query.QueryBuilder query) {
@@ -70,7 +70,7 @@ private void addToPreJoinFilters(org.elasticsearch.index.query.QueryBuilder quer
         }
     }
 
-    private void buildPrePostJoinFilter(PhysicalPlan rightPreJoinPlan, ClusterService clusterService) {
+    private void buildPreJoinFilter(PhysicalPlan rightPreJoinPlan, ClusterService clusterService) {
         if (rightPreJoinPlan instanceof EsQueryExec esQueryExec) {
             // this does not happen right now, as we only do local mapping on the lookup node
             // so we have EsSourceExec, not esQueryExec
@@ -79,12 +79,12 @@ private void buildPrePostJoinFilter(PhysicalPlan rightPreJoinPlan, ClusterServic
             }
         } else if (rightPreJoinPlan instanceof FilterExec filterExec) {
             List<Expression> candidateRightHandFilters = Predicates.splitAnd(filterExec.condition());
+            LucenePushdownPredicates lucenePushdownPredicates = LucenePushdownPredicates.from(
+                SearchContextStats.from(List.of(context)),
+                new EsqlFlags(clusterService.getClusterSettings())
+            );
             for (Expression filter : candidateRightHandFilters) {
                 if (filter instanceof TranslationAware translationAware) {
-                    LucenePushdownPredicates lucenePushdownPredicates = LucenePushdownPredicates.from(
-                        SearchContextStats.from(List.of(context)),
-                        new EsqlFlags(clusterService.getClusterSettings())
-                    );
                     if (TranslationAware.Translatable.YES.equals(translationAware.translatable(lucenePushdownPredicates))) {
                         addToPreJoinFilters(translationAware.asQuery(lucenePushdownPredicates, TRANSLATOR_HANDLER).toQueryBuilder());
                     }
@@ -96,10 +96,10 @@ private void buildPrePostJoinFilter(PhysicalPlan rightPreJoinPlan, ClusterServic
             }
             // call recursively to find other filters that might be present
             // either in another FilterExec or in an EsQueryExec
-            buildPrePostJoinFilter(filterExec.child(), clusterService);
+            buildPreJoinFilter(filterExec.child(), clusterService);
         } else if (rightPreJoinPlan instanceof UnaryExec unaryExec) {
             // there can be other nodes in the plan such as FieldExtractExec in the future
-            buildPrePostJoinFilter(unaryExec.child(), clusterService);
+            buildPreJoinFilter(unaryExec.child(), clusterService);
         }
         // else we do nothing, as the filters are optional and we don't want to fail the query if there are any errors
         // this also covers the case of rightPreJoinPlan being null
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java
@@ -123,7 +123,7 @@ protected LookupEnrichQueryGenerator queryList(
 
         PhysicalPlan physicalPlan = request.rightPreJoinPlan;
         physicalPlan = localLookupNodePlanning(physicalPlan);
-        if (queryLists.size() == 1 && (physicalPlan instanceof FilterExec == false)) {
+        if (queryLists.size() == 1 && physicalPlan instanceof FilterExec == false) {
             return queryLists.getFirst();
         }
         return new ExpressionQueryList(queryLists, context, physicalPlan, clusterService);
@@ -135,7 +135,7 @@ protected LookupEnrichQueryGenerator queryList(
      * For now, we will just do mapping of the logical plan to physical plan
      * In the future we can also do local physical and logical optimizations
      */
-    private PhysicalPlan localLookupNodePlanning(PhysicalPlan physicalPlan) {
+    private static PhysicalPlan localLookupNodePlanning(PhysicalPlan physicalPlan) {
         if (physicalPlan instanceof FragmentExec fragmentExec) {
             try {
                 LocalMapper localMapper = new LocalMapper();
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFilters.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFilters.java
@@ -168,11 +168,15 @@ private static LogicalPlan pushDownPastJoin(Filter filter, Join join, FoldContex
                 if (rightPushableFilters.isEmpty() == false) {
                     if (join.right() instanceof Filter existingRightFilter) {
                         // merge the unique AND filter components from rightPushableFilters and existingRightFilter.condition()
+
                         List<Expression> existingFilters = new ArrayList<>(Predicates.splitAnd(existingRightFilter.condition()));
+                        int sizeBefore = existingFilters.size();
                         rightPushableFilters.stream().filter(e -> existingFilters.contains(e) == false).forEach(existingFilters::add);
-                        right = existingRightFilter.with(Predicates.combineAnd(existingFilters));
-                        join = (Join) join.replaceRight(right);
-                        optimizationApplied = true;
+                        if (sizeBefore != existingFilters.size()) {
+                            right = existingRightFilter.with(Predicates.combineAnd(existingFilters));
+                            join = (Join) join.replaceRight(right);
+                            optimizationApplied = true;
+                        } // else nothing needs to be updated
                     } else {
                         // create a new filter on top of the right child
                         right = new Filter(right.source(), right, Predicates.combineAnd(rightPushableFilters));
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlFlags.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlFlags.java
@@ -41,7 +41,7 @@ public class EsqlFlags {
         Setting.Property.NodeScope,
         Setting.Property.Dynamic
     );
-    public static List<Setting<?>> allEsqlFlagsSettings = List.of(ESQL_STRING_LIKE_ON_INDEX, ESQL_ROUNDTO_PUSHDOWN_THRESHOLD);
+    public static List<Setting<?>> ALL_ESQL_FLAGS_SETTINGS = List.of(ESQL_STRING_LIKE_ON_INDEX, ESQL_ROUNDTO_PUSHDOWN_THRESHOLD);
     private final boolean stringLikeOnIndex;
 
     private final int roundToPushdownThreshold;
@@ -82,8 +82,4 @@ public boolean stringLikeOnIndex() {
     public int roundToPushdownThreshold() {
         return roundToPushdownThreshold;
     }
-
-    public static List<Setting<?>> listAll() {
-        return allEsqlFlagsSettings;
-    }
 }
diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexOperatorTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexOperatorTests.java
@@ -257,7 +257,7 @@ private LookupFromIndexService lookupService(DriverContext mainContext) {
         boolean beCranky = mainContext.bigArrays().breakerService() instanceof CrankyCircuitBreakerService;
         DiscoveryNode localNode = DiscoveryNodeUtils.create("node", "node");
         var builtInClusterSettings = new HashSet<>(ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
-        builtInClusterSettings.addAll(EsqlFlags.listAll());
+        builtInClusterSettings.addAll(EsqlFlags.ALL_ESQL_FLAGS_SETTINGS);
         ClusterService clusterService = ClusterServiceUtils.createClusterService(
             threadPool,
             localNode,

Original file line number	Diff line number	Diff line change
`@@ -387,7 +387,7 @@ public Query regexpQuery(`
`387`	`387`	`}`
`388`	`388`
`389`	`389`	`/**`
`390`		`- * Returns a Lucine pushable Query for the current field`
	`390`	`+ * Returns a Lucene pushable Query for the current field`
`391`	`391`	`* For now can only be AutomatonQuery or MatchAllDocsQuery() or MatchNoDocsQuery()`
`392`	`392`	`*/`
`393`	`393`	`public Query automatonQuery(`
Original file line number	Diff line number	Diff line change
`@@ -774,7 +774,7 @@ private Map<String, Object> lookupExplosion(int sensorDataCount, int lookupEntri`
`774`	`774`	`}`
`775`	`775`	`if (lookupEntries != lookupEntriesToKeep) {`
`776`	`776`	`// add a filter to reduce the number of matches`
`777`		`- // we add both a Lucine pushable filter and a non-pushable filter`
	`777`	`+ // we add both a Lucene pushable filter and a non-pushable filter`
`778`	`778`	`// this is to make sure that even if there are non-pushable filters the pushable filters is still applied`
`779`	`779`	`query.append(" \| WHERE ABS(filter_key) > -1 AND filter_key < ").append(lookupEntriesToKeep);`
`780`	`780`
Original file line number	Diff line number	Diff line change
`@@ -159,7 +159,7 @@ Page buildPage(int positions, IntVector.Builder positionsBuilder, IntVector.Buil`
`159`	`159`	`return page;`
`160`	`160`	`}`
`161`	`161`
`162`		`- private Query nextQuery() throws IOException {`
	`162`	`+ private Query nextQuery() {`
`163`	`163`	`++queryPosition;`
`164`	`164`	`while (isFinished() == false) {`
`165`	`165`	`Query query = queryList.getQuery(queryPosition);`