ESQL: Don't mutate the BoolQueryBuilder in plan (backport of elastic#111519) (elastic#112679)

* ESQL: Don't mutate the BoolQueryBuilder in plan (elastic#111519)

This modifies ESQL's `QueryBuilder` merging to stop it from mutating
`BoolQueryBuilder`s in place. It's more efficient when you can do that,
but only marginally so. Instead we create a shallow copy of the same
builder and mutate *that*. That lines up much better with the plan being
immutable objects. It should be!

The resulting queries that ESQL sends to lucene are the same here - we
just modify how we build them.

This should stop a fun class of bugs that can come up when we mutate the
query builders in multiple threads - because we *do* replan the query in
multiple threads. That's fine. So long as we shallow copy, like we do in
this PR.

* Update docs/changelog/112679.yaml

* Delete docs/changelog/112679.yaml

Author: nik9000

docs/changelog/111519.yaml

@@ -0,0 +1,5 @@
+pr: 111519
+summary: "ESQL: Don't mutate the `BoolQueryBuilder` in plan"
+area: ES|QL
+type: bug
+issues: []

server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java

@@ -410,4 +410,28 @@ private static boolean rewriteClauses(
     public TransportVersion getMinimalSupportedVersion() {
         return TransportVersions.ZERO;
     }
+
+    /**
+     * Create a new builder with the same clauses but modification of
+     * the builder won't modify the original. Modifications of any
+     * of the copy's clauses will modify the original. Don't so that.
+     */
+    public BoolQueryBuilder shallowCopy() {
+        BoolQueryBuilder copy = new BoolQueryBuilder();
+        copy.adjustPureNegative = adjustPureNegative;
+        copy.minimumShouldMatch = minimumShouldMatch;
+        for (QueryBuilder q : mustClauses) {
+            copy.must(q);
+        }
+        for (QueryBuilder q : mustNotClauses) {
+            copy.mustNot(q);
+        }
+        for (QueryBuilder q : filterClauses) {
+            copy.filter(q);
+        }
+        for (QueryBuilder q : shouldClauses) {
+            copy.should(q);
+        }
+        return copy;
+    }
 }

server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java

@@ -32,7 +32,9 @@
 import static org.elasticsearch.index.query.QueryBuilders.termQuery;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.Matchers.not;
 
 public class BoolQueryBuilderTests extends AbstractQueryTestCase<BoolQueryBuilder> {
     @Override
@@ -463,4 +465,39 @@ public void testMustRewrite() throws IOException {
         IllegalStateException e = expectThrows(IllegalStateException.class, () -> boolQuery.toQuery(context));
         assertEquals("Rewrite first", e.getMessage());
     }
+
+    public void testShallowCopy() {
+        BoolQueryBuilder orig = createTestQueryBuilder();
+        BoolQueryBuilder shallowCopy = orig.shallowCopy();
+        assertThat(shallowCopy.adjustPureNegative(), equalTo(orig.adjustPureNegative()));
+        assertThat(shallowCopy.minimumShouldMatch(), equalTo(orig.minimumShouldMatch()));
+        assertThat(shallowCopy.must(), equalTo(orig.must()));
+        assertThat(shallowCopy.mustNot(), equalTo(orig.mustNot()));
+        assertThat(shallowCopy.should(), equalTo(orig.should()));
+        assertThat(shallowCopy.filter(), equalTo(orig.filter()));
+
+        QueryBuilder b = new MatchQueryBuilder("foo", "bar");
+        switch (between(0, 3)) {
+            case 0 -> {
+                shallowCopy.must(b);
+                assertThat(shallowCopy.must(), hasItem(b));
+                assertThat(orig.must(), not(hasItem(b)));
+            }
+            case 1 -> {
+                shallowCopy.mustNot(b);
+                assertThat(shallowCopy.mustNot(), hasItem(b));
+                assertThat(orig.mustNot(), not(hasItem(b)));
+            }
+            case 2 -> {
+                shallowCopy.should(b);
+                assertThat(shallowCopy.should(), hasItem(b));
+                assertThat(orig.should(), not(hasItem(b)));
+            }
+            case 3 -> {
+                shallowCopy.filter(b);
+                assertThat(shallowCopy.filter(), hasItem(b));
+                assertThat(orig.filter(), not(hasItem(b)));
+            }
+        }
+    }
 }

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/Queries.java

@@ -50,7 +50,7 @@ public static QueryBuilder combine(Clause clause, List<QueryBuilder> queries) {
             if (firstQuery == null) {
                 firstQuery = query;
                 if (firstQuery instanceof BoolQueryBuilder bqb) {
-                    bool = bqb;
+                    bool = bqb.shallowCopy();
                 }
             }
             // at least two entries, start copying

x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/util/QueriesTests.java

@@ -14,6 +14,7 @@
 
 import static java.util.Arrays.asList;
 import static org.hamcrest.Matchers.everyItem;
+import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.in;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.sameInstance;
@@ -92,13 +93,27 @@ public void testCombineBoolQueries() {
 
         assertThat(combination, instanceOf(BoolQueryBuilder.class));
         var bool = (BoolQueryBuilder) combination;
+        assertBoolQueryMerge(queries, bool, clause);
+    }
 
-        var clauseList = clause.innerQueries.apply(bool);
+    private void assertBoolQueryMerge(QueryBuilder[] queries, BoolQueryBuilder bool, Queries.Clause clause) {
+        BoolQueryBuilder first = (BoolQueryBuilder) queries[0];
+        for (QueryBuilder b : first.must()) {
+            assertThat(bool.must(), hasItem(b));
+        }
+        for (QueryBuilder b : first.mustNot()) {
+            assertThat(bool.mustNot(), hasItem(b));
+        }
+        for (QueryBuilder b : first.should()) {
+            assertThat(bool.should(), hasItem(b));
+        }
+        for (QueryBuilder b : first.filter()) {
+            assertThat(bool.filter(), hasItem(b));
+        }
 
-        for (QueryBuilder query : queries) {
-            if (query != bool) {
-                assertThat(query, in(clauseList));
-            }
+        var clauseList = clause.innerQueries.apply(bool);
+        for (int i = 1; i < queries.length; i++) {
+            assertThat(queries[i], in(clauseList));
         }
     }
 
@@ -118,10 +133,11 @@ public void testCombineMixedBoolAndNonBoolQueries() {
         assertThat(combination, instanceOf(BoolQueryBuilder.class));
         var bool = (BoolQueryBuilder) combination;
 
-        var clauseList = clause.innerQueries.apply(bool);
-
-        for (QueryBuilder query : queries) {
-            if (query != bool) {
+        if (queries[0] instanceof BoolQueryBuilder) {
+            assertBoolQueryMerge(queries, bool, clause);
+        } else {
+            var clauseList = clause.innerQueries.apply(bool);
+            for (QueryBuilder query : queries) {
                 assertThat(query, in(clauseList));
             }
         }

x-pack/plugin/esql/qa/server/multi-node/build.gradle

@@ -8,6 +8,9 @@ dependencies {
   javaRestTestImplementation project(xpackModule('esql:qa:testFixtures'))
   javaRestTestImplementation project(xpackModule('esql:qa:server'))
   yamlRestTestImplementation project(xpackModule('esql:qa:server'))
+
+  clusterPlugins project(':plugins:mapper-size')
+  clusterPlugins project(':plugins:mapper-murmur3')
 }
 
 GradleUtils.extendSourceSet(project, "javaRestTest", "yamlRestTest")

x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/Clusters.java

@@ -8,15 +8,17 @@
 package org.elasticsearch.xpack.esql.qa.multi_node;
 
 import org.elasticsearch.test.cluster.ElasticsearchCluster;
+import org.elasticsearch.test.cluster.local.LocalClusterConfigProvider;
 import org.elasticsearch.test.cluster.local.distribution.DistributionType;
 
 public class Clusters {
-    public static ElasticsearchCluster testCluster() {
+    public static ElasticsearchCluster testCluster(LocalClusterConfigProvider configProvider) {
         return ElasticsearchCluster.local()
             .distribution(DistributionType.DEFAULT)
             .nodes(2)
             .setting("xpack.security.enabled", "false")
             .setting("xpack.license.self_generated.type", "trial")
+            .apply(() -> configProvider)
             .build();
     }
 }

x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java

@@ -14,7 +14,7 @@
 
 public class EsqlSpecIT extends EsqlSpecTestCase {
     @ClassRule
-    public static ElasticsearchCluster cluster = Clusters.testCluster();
+    public static ElasticsearchCluster cluster = Clusters.testCluster(spec -> {});
 
     @Override
     protected String getTestRestCluster() {

x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/FieldExtractorIT.java

@@ -17,7 +17,7 @@
 @ThreadLeakFilters(filters = TestClustersThreadFilter.class)
 public class FieldExtractorIT extends FieldExtractorTestCase {
     @ClassRule
-    public static ElasticsearchCluster cluster = Clusters.testCluster();
+    public static ElasticsearchCluster cluster = Clusters.testCluster(spec -> {});
 
     @Override
     protected String getTestRestCluster() {

x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/RestEsqlIT.java

@@ -0,0 +1,38 @@
+/*
+ * 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.qa.multi_node;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.elasticsearch.test.cluster.ElasticsearchCluster;
+import org.elasticsearch.xpack.esql.qa.rest.RestEsqlTestCase;
+import org.junit.ClassRule;
+
+import java.util.Arrays;
+import java.util.List;
+
+public class RestEsqlIT extends RestEsqlTestCase {
+    @ClassRule
+    public static ElasticsearchCluster cluster = Clusters.testCluster(
+        specBuilder -> specBuilder.plugin("mapper-size").plugin("mapper-murmur3")
+    );
+
+    @Override
+    protected String getTestRestCluster() {
+        return cluster.getHttpAddresses();
+    }
+
+    @ParametersFactory(argumentFormatting = "%1s")
+    public static List<Object[]> modes() {
+        return Arrays.stream(Mode.values()).map(m -> new Object[] { m }).toList();
+    }
+
+    public RestEsqlIT(Mode mode) {
+        super(mode);
+    }
+}