Update per comments

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

Author: atris

server/src/main/java/org/opensearch/search/SearchService.java

@@ -529,6 +529,10 @@ public SearchService(
         this.concurrentSearchDeciderFactories = concurrentSearchDeciderFactories;
 
         this.pluginProfilers = pluginProfilers;
+
+        // Initialize QueryRewriterRegistry with cluster settings so TermsMergingRewriter
+        // can register its settings update consumer
+        QueryRewriterRegistry.initialize(settings, clusterService.getClusterSettings());
     }
 
     private void validateKeepAlives(TimeValue defaultKeepAlive, TimeValue maxKeepAlive) {
@@ -1514,8 +1518,7 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
 
             // Apply query rewriting optimizations if enabled
             if (QUERY_REWRITING_ENABLED_SETTING.get(clusterService.getSettings())) {
-                int termsThreshold = QUERY_REWRITING_TERMS_THRESHOLD_SETTING.get(clusterService.getSettings());
-                query = QueryRewriterRegistry.rewrite(query, queryShardContext, true, termsThreshold);
+                query = QueryRewriterRegistry.rewrite(query, queryShardContext, true);
             }
 
             InnerHitContextBuilder.extractInnerHits(query, innerHitBuilders);

server/src/main/java/org/opensearch/search/query/QueryRewriterRegistry.java

@@ -10,6 +10,8 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.opensearch.common.settings.ClusterSettings;
+import org.opensearch.common.settings.Settings;
 import org.opensearch.index.query.QueryBuilder;
 import org.opensearch.index.query.QueryShardContext;
 import org.opensearch.search.query.rewriters.BooleanFlatteningRewriter;
@@ -40,15 +42,23 @@ public final class QueryRewriterRegistry {
      */
     private final CopyOnWriteArrayList<QueryRewriter> rewriters;
 
+    /**
+     * TermsMergingRewriter instance that needs settings initialization.
+     */
+    private final TermsMergingRewriter termsMergingRewriter;
+
     private QueryRewriterRegistry() {
         this.rewriters = new CopyOnWriteArrayList<>();
 
         // Register default rewriters
-        // Note: TermsMergingRewriter is special - it needs threshold at runtime
         registerRewriter(new BooleanFlatteningRewriter());
         registerRewriter(new MustToFilterRewriter());
         registerRewriter(new MustNotToShouldRewriter());
         registerRewriter(new MatchAllRemovalRewriter());
+
+        // TermsMergingRewriter is registered as singleton
+        this.termsMergingRewriter = new TermsMergingRewriter();
+        registerRewriter(termsMergingRewriter);
     }
 
     /**
@@ -71,28 +81,25 @@ public void registerRewriter(QueryRewriter rewriter) {
     }
 
     /**
-     * Get a list of all rewriters with the given terms threshold.
+     * Initialize the registry with cluster settings.
+     * This must be called once during system startup to properly configure
+     * the TermsMergingRewriter with settings and update consumers.
+     *
+     * @param settings Initial cluster settings
+     * @param clusterSettings Cluster settings for registering update consumers
      */
-    private List<QueryRewriter> getRewritersWithThreshold(int termsThreshold) {
-        List<QueryRewriter> allRewriters = new ArrayList<>(rewriters);
-        // Add TermsMergingRewriter with the current threshold
-        // This is added dynamically because it needs the threshold parameter
-        allRewriters.add(new TermsMergingRewriter(termsThreshold));
-        allRewriters.sort(Comparator.comparingInt(QueryRewriter::priority));
-        return allRewriters;
+    public static void initialize(Settings settings, ClusterSettings clusterSettings) {
+        getInstance().termsMergingRewriter.initialize(settings, clusterSettings);
     }
 
     public static QueryBuilder rewrite(QueryBuilder query, QueryShardContext context, boolean enabled) {
-        return rewrite(query, context, enabled, 16);
-    }
-
-    public static QueryBuilder rewrite(QueryBuilder query, QueryShardContext context, boolean enabled, int termsThreshold) {
         if (!enabled || query == null) {
             return query;
         }
 
         QueryRewriterRegistry registry = getInstance();
-        List<QueryRewriter> sortedRewriters = registry.getRewritersWithThreshold(termsThreshold);
+        List<QueryRewriter> sortedRewriters = new ArrayList<>(registry.rewriters);
+        sortedRewriters.sort(Comparator.comparingInt(QueryRewriter::priority));
 
         QueryBuilder current = query;
         for (QueryRewriter rewriter : sortedRewriters) {

server/src/main/java/org/opensearch/search/query/rewriters/MatchAllRemovalRewriter.java

@@ -9,7 +9,6 @@
 package org.opensearch.search.query.rewriters;
 
 import org.opensearch.index.query.BoolQueryBuilder;
-import org.opensearch.index.query.ConstantScoreQueryBuilder;
 import org.opensearch.index.query.MatchAllQueryBuilder;
 import org.opensearch.index.query.QueryBuilder;
 import org.opensearch.index.query.QueryShardContext;
@@ -43,11 +42,8 @@ private QueryBuilder rewriteBoolQuery(BoolQueryBuilder original) {
                 if (q instanceof MatchAllQueryBuilder) {
                     matchAllCount++;
                     matchAllInMust++;
-                } else if (q instanceof ConstantScoreQueryBuilder) {
-                    // Don't treat constant score queries as match_all even if they contain match_all
-                    onlyMatchAll = false;
-                    break;
                 } else {
+                    // Don't treat constant score queries or any other queries as match_all
                     onlyMatchAll = false;
                     break;
                 }
@@ -83,15 +79,18 @@ private QueryBuilder rewriteBoolQuery(BoolQueryBuilder original) {
             return original;
         }
 
-        // Create rewritten query
+        // Clone the query structure
         BoolQueryBuilder rewritten = new BoolQueryBuilder();
         rewritten.boost(original.boost());
         rewritten.queryName(original.queryName());
         rewritten.minimumShouldMatch(original.minimumShouldMatch());
         rewritten.adjustPureNegative(original.adjustPureNegative());
 
-        // Process all clauses
-        // For must clauses, only remove match_all if there are other non-match_all queries
+        // Process each clause type with different match_all removal logic:
+        // - must: Remove match_all only if other queries exist (preserves scoring semantics)
+        // - filter: Always remove match_all (it's redundant in non-scoring context)
+        // - should: Keep match_all (changes OR semantics if removed)
+        // - mustNot: Keep match_all (excluding all docs is meaningful)
         processClausesWithContext(original.must(), rewritten::must, true, original, true);
         processClauses(original.filter(), rewritten::filter, true, original);
         processClauses(original.should(), rewritten::should, false, original);

server/src/main/java/org/opensearch/search/query/rewriters/TermsMergingRewriter.java

@@ -8,11 +8,14 @@
 
 package org.opensearch.search.query.rewriters;
 
+import org.opensearch.common.settings.ClusterSettings;
+import org.opensearch.common.settings.Settings;
 import org.opensearch.index.query.BoolQueryBuilder;
 import org.opensearch.index.query.QueryBuilder;
 import org.opensearch.index.query.QueryShardContext;
 import org.opensearch.index.query.TermQueryBuilder;
 import org.opensearch.index.query.TermsQueryBuilder;
+import org.opensearch.search.SearchService;
 import org.opensearch.search.query.QueryRewriter;
 
 import java.util.ArrayList;
@@ -55,24 +58,28 @@ public class TermsMergingRewriter implements QueryRewriter {
     private static final int DEFAULT_MINIMUM_TERMS_TO_MERGE = 16;
 
     /**
-     * The minimum number of terms to merge for this instance.
+     * The minimum number of terms to merge.
      */
-    private final int minimumTermsToMerge;
+    private volatile int minimumTermsToMerge = DEFAULT_MINIMUM_TERMS_TO_MERGE;
 
     /**
-     * Creates a new rewriter with the default threshold.
+     * Creates a new rewriter.
      */
-    public TermsMergingRewriter() {
-        this(DEFAULT_MINIMUM_TERMS_TO_MERGE);
-    }
+    public TermsMergingRewriter() {}
 
     /**
-     * Creates a new rewriter with a custom threshold.
+     * Initialize this rewriter with cluster settings.
+     * This registers an update consumer to keep the threshold in sync with the cluster setting.
      *
-     * @param minimumTermsToMerge The minimum number of terms to merge
+     * @param settings Initial settings
+     * @param clusterSettings Cluster settings to register update consumer
      */
-    public TermsMergingRewriter(int minimumTermsToMerge) {
-        this.minimumTermsToMerge = minimumTermsToMerge;
+    public void initialize(Settings settings, ClusterSettings clusterSettings) {
+        this.minimumTermsToMerge = SearchService.QUERY_REWRITING_TERMS_THRESHOLD_SETTING.get(settings);
+        clusterSettings.addSettingsUpdateConsumer(
+            SearchService.QUERY_REWRITING_TERMS_THRESHOLD_SETTING,
+            threshold -> this.minimumTermsToMerge = threshold
+        );
     }
 
     @Override

server/src/test/java/org/opensearch/search/query/rewriters/TermsMergingRewriterTests.java

@@ -22,7 +22,6 @@
 public class TermsMergingRewriterTests extends OpenSearchTestCase {
 
     private final TermsMergingRewriter rewriter = new TermsMergingRewriter();
-    private final TermsMergingRewriter customThresholdRewriter = new TermsMergingRewriter(5);
     private final QueryShardContext context = mock(QueryShardContext.class);
 
     public void testSimpleTermMergingBelowThreshold() {
@@ -290,27 +289,4 @@ public void testMixedTermsAndTermQueriesAboveThreshold() {
         assertThat(merged.values().size(), equalTo(20));
     }
 
-    public void testCustomThreshold() {
-        // Test with custom threshold of 5
-        BoolQueryBuilder query = QueryBuilders.boolQuery();
-
-        // Add 6 term queries (above custom threshold of 5)
-        for (int i = 0; i < 6; i++) {
-            query.filter(QueryBuilders.termQuery("field", "value" + i));
-        }
-
-        // Should merge with custom threshold rewriter
-        QueryBuilder rewritten = customThresholdRewriter.rewrite(query, context);
-        assertThat(rewritten, instanceOf(BoolQueryBuilder.class));
-        BoolQueryBuilder rewrittenBool = (BoolQueryBuilder) rewritten;
-
-        assertThat(rewrittenBool.filter().size(), equalTo(1));
-        assertThat(rewrittenBool.filter().get(0), instanceOf(TermsQueryBuilder.class));
-        TermsQueryBuilder termsQuery = (TermsQueryBuilder) rewrittenBool.filter().get(0);
-        assertThat(termsQuery.values().size(), equalTo(6));
-
-        // Should NOT merge with default threshold rewriter (6 < 16)
-        QueryBuilder defaultRewritten = rewriter.rewrite(query, context);
-        assertSame(query, defaultRewritten); // No changes expected
-    }
 }