Replace script-query blacklist with fail-closed allow-list for efficient filter

The prior containsScriptQuery walker missed any wrapper type it didn't know about, which risked silently embedding unsupported queries (e.g., NestedQueryBuilder) under knn.filter. Switch to an allow-list validator over FilterQueryBuilder's actual leaf output (term/range/wildcard/match*/query_string/simple_query_string/match_bool_prefix/exists), recurse bool and constant_score, reject script and nested with targeted messages, and fail closed on unknown shapes.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

Author: mengweieric

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/VectorSearchQueryBuilder.java

@@ -6,14 +6,27 @@
 package org.opensearch.sql.opensearch.storage.scan;
 
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Function;
 import org.apache.commons.lang3.tuple.Pair;
 import org.opensearch.index.query.BoolQueryBuilder;
 import org.opensearch.index.query.ConstantScoreQueryBuilder;
+import org.opensearch.index.query.ExistsQueryBuilder;
+import org.opensearch.index.query.MatchBoolPrefixQueryBuilder;
+import org.opensearch.index.query.MatchPhrasePrefixQueryBuilder;
+import org.opensearch.index.query.MatchPhraseQueryBuilder;
+import org.opensearch.index.query.MatchQueryBuilder;
+import org.opensearch.index.query.MultiMatchQueryBuilder;
+import org.opensearch.index.query.NestedQueryBuilder;
 import org.opensearch.index.query.QueryBuilder;
 import org.opensearch.index.query.QueryBuilders;
+import org.opensearch.index.query.QueryStringQueryBuilder;
+import org.opensearch.index.query.RangeQueryBuilder;
 import org.opensearch.index.query.ScriptQueryBuilder;
+import org.opensearch.index.query.SimpleQueryStringBuilder;
+import org.opensearch.index.query.TermQueryBuilder;
+import org.opensearch.index.query.WildcardQueryBuilder;
 import org.opensearch.sql.ast.tree.Sort;
 import org.opensearch.sql.ast.tree.Sort.SortOption;
 import org.opensearch.sql.exception.ExpressionEvaluationException;
@@ -83,11 +96,8 @@ public boolean pushDownFilter(LogicalFilter filter) {
     FilterQueryBuilder queryBuilder = new FilterQueryBuilder(new DefaultExpressionSerializer());
     Expression queryCondition = filter.getCondition();
 
-    // Reject WHERE predicates that reference the synthetic _score column. The planner surfaces
-    // _score as a projectable/filterable field, but it is not a stored document field in
-    // OpenSearch. If we let this pass through, FilterQueryBuilder produces a range query on a
-    // non-existent field and the cluster silently returns 0 rows. Users who want a score floor
-    // should use option='min_score=...' instead.
+    // _score is synthetic, not a stored field; a range query on it silently returns 0 rows.
+    // Users who want a score floor should use option='min_score=...'.
     if (containsScoreReference(queryCondition)) {
       throw new ExpressionEvaluationException(
           "WHERE on _score is not supported on vectorSearch()."
@@ -109,16 +119,9 @@ public boolean pushDownFilter(LogicalFilter filter) {
     filterPushed = true;
 
     if (filterType == FilterType.EFFICIENT) {
-      // knn.filter on AOSS/serverless vector collections rejects script queries. If any WHERE
-      // subtree compiled to a ScriptQueryBuilder (arithmetic, function calls, CASE, date math),
-      // refuse to embed it under knn.filter instead of shipping a request that will fail at the
-      // cluster with an opaque error.
-      if (containsScriptQuery(whereQuery)) {
-        throw new ExpressionEvaluationException(
-            "filter_type=efficient does not support predicates that compile to script queries"
-                + " (arithmetic, function calls, CASE, date math). Rewrite the WHERE clause to"
-                + " use comparable/term/range predicates, or omit filter_type.");
-      }
+      // Fail closed: knn.filter on AOSS rejects script queries and nested predicates expand the
+      // preview contract. Allow-list validator beats a blacklist walker.
+      validateEfficientFilterSafe(whereQuery);
       QueryBuilder rebuiltKnn = rebuildKnnWithFilter.apply(whereQuery);
       requestBuilder.getSourceBuilder().query(rebuiltKnn);
     } else {
@@ -131,10 +134,8 @@ public boolean pushDownFilter(LogicalFilter filter) {
 
   @Override
   public boolean pushDownLimit(LogicalLimit limit) {
-    // OFFSET on vectorSearch() silently rewrites the search window and drops top results, which
-    // defeats the entire point of a relevance-ranked top-k query. The parent path would push
-    // `from: <offset>` into the OpenSearch request; reject it explicitly so users get a clear
-    // error instead of surprising result shifts.
+    // OFFSET would shift the search window and silently drop top results; reject with a clear
+    // error rather than have the parent path push `from: <offset>` into the request.
     if (limit.getOffset() != null && limit.getOffset() != 0) {
       throw new ExpressionEvaluationException(
           "OFFSET is not supported on vectorSearch(). Remove OFFSET and use LIMIT only.");
@@ -163,9 +164,8 @@ public boolean pushDownSort(LogicalSort sort) {
             "vectorSearch only supports ORDER BY _score DESC; _score ASC is not supported");
       }
     }
-    // _score DESC is the natural knn order — no need to push the sort itself to OpenSearch.
-    // Preserve the parent's sort.getCount() → limit pushdown contract: SQL always sets count=0,
-    // but PPL or future callers may set a non-zero count to combine sort+limit in one node.
+    // _score DESC is knn's natural order, so the sort itself is not pushed. Preserve the
+    // parent's sort.getCount() → limit contract; SQL sends 0, PPL may combine sort+limit.
     if (sort.getCount() != 0) {
       validateLimitWithinK(sort.getCount());
       limitPushed = true;
@@ -185,20 +185,14 @@ private void validateLimitWithinK(int limit) {
     }
   }
 
-  /**
-   * Returns true if any subexpression is a ReferenceExpression whose attr is "_score". Uses the
-   * standard ExpressionNodeVisitor so compound predicates (AND/OR/NOT, function calls, CASE) are
-   * walked uniformly.
-   */
+  // True if any ReferenceExpression in the tree names _score (case-insensitive, so quoted/
+  // backticked variants cannot bypass the guard).
   private static boolean containsScoreReference(Expression expr) {
     AtomicBoolean found = new AtomicBoolean(false);
     expr.accept(
         new ExpressionNodeVisitor<Void, Void>() {
           @Override
           public Void visitReference(ReferenceExpression node, Void context) {
-            // Case-insensitive match so _SCORE, _Score, and any quoted/backticked variant that
-            // preserves original casing cannot bypass the guard and reach the cluster as a range
-            // query on a non-existent field.
             if (node.getAttr() != null && "_score".equalsIgnoreCase(node.getAttr())) {
               found.set(true);
             }
@@ -209,48 +203,60 @@ public Void visitReference(ReferenceExpression node, Void context) {
     return found.get();
   }
 
-  /**
-   * Recursively scans a QueryBuilder tree for any ScriptQueryBuilder. Handles the common wrappers
-   * that FilterQueryBuilder produces: BoolQueryBuilder (must/should/mustNot/filter) and
-   * ConstantScoreQueryBuilder. Other QueryBuilder subtypes are leaves for our purposes: if the
-   * top-level builder itself is a ScriptQueryBuilder we catch it, otherwise we treat it as
-   * script-free.
-   */
-  private static boolean containsScriptQuery(QueryBuilder qb) {
+  // Allow-list of leaf query types FilterQueryBuilder emits today. Any new wrapper or container
+  // appearing here must fail closed rather than silently embed under knn.filter.
+  private static final Set<Class<? extends QueryBuilder>> SAFE_EFFICIENT_FILTER_LEAVES =
+      Set.of(
+          TermQueryBuilder.class,
+          RangeQueryBuilder.class,
+          WildcardQueryBuilder.class,
+          MatchQueryBuilder.class,
+          MatchPhraseQueryBuilder.class,
+          MatchPhrasePrefixQueryBuilder.class,
+          MultiMatchQueryBuilder.class,
+          QueryStringQueryBuilder.class,
+          SimpleQueryStringBuilder.class,
+          MatchBoolPrefixQueryBuilder.class,
+          ExistsQueryBuilder.class);
+
+  // Package-private for direct branch coverage in unit tests. Fail-closed: recurse known
+  // containers, reject ScriptQueryBuilder/NestedQueryBuilder with targeted messages, allow
+  // listed leaves, reject everything else as unsupported shape.
+  static void validateEfficientFilterSafe(QueryBuilder qb) {
     if (qb == null) {
-      return false;
+      return;
     }
     if (qb instanceof ScriptQueryBuilder) {
-      return true;
+      throw new ExpressionEvaluationException(
+          "filter_type=efficient does not support predicates that compile to script queries"
+              + " (arithmetic, function calls, CASE, date math). Rewrite the WHERE clause to"
+              + " use comparable/term/range predicates, or omit filter_type.");
     }
     if (qb instanceof BoolQueryBuilder) {
       BoolQueryBuilder bool = (BoolQueryBuilder) qb;
-      for (QueryBuilder child : bool.must()) {
-        if (containsScriptQuery(child)) {
-          return true;
-        }
-      }
-      for (QueryBuilder child : bool.filter()) {
-        if (containsScriptQuery(child)) {
-          return true;
-        }
-      }
-      for (QueryBuilder child : bool.should()) {
-        if (containsScriptQuery(child)) {
-          return true;
-        }
-      }
-      for (QueryBuilder child : bool.mustNot()) {
-        if (containsScriptQuery(child)) {
-          return true;
-        }
-      }
-      return false;
+      bool.must().forEach(VectorSearchQueryBuilder::validateEfficientFilterSafe);
+      bool.filter().forEach(VectorSearchQueryBuilder::validateEfficientFilterSafe);
+      bool.should().forEach(VectorSearchQueryBuilder::validateEfficientFilterSafe);
+      bool.mustNot().forEach(VectorSearchQueryBuilder::validateEfficientFilterSafe);
+      return;
     }
     if (qb instanceof ConstantScoreQueryBuilder) {
-      return containsScriptQuery(((ConstantScoreQueryBuilder) qb).innerQuery());
+      validateEfficientFilterSafe(((ConstantScoreQueryBuilder) qb).innerQuery());
+      return;
+    }
+    if (qb instanceof NestedQueryBuilder) {
+      throw new ExpressionEvaluationException(
+          "filter_type=efficient does not support nested predicates in this preview."
+              + " Rewrite the WHERE clause using non-nested fields or omit filter_type.");
+    }
+    if (SAFE_EFFICIENT_FILTER_LEAVES.contains(qb.getClass())) {
+      return;
     }
-    return false;
+    throw new ExpressionEvaluationException(
+        "filter_type=efficient encountered an unsupported filter query shape: "
+            + qb.getClass().getSimpleName()
+            + ". Rewrite the WHERE clause using simple term/range/bool predicates,"
+            + " or omit filter_type.");
   }
 
   @Override

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/VectorSearchQueryBuilderTest.java

@@ -17,9 +17,11 @@
 import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
+import org.apache.lucene.search.join.ScoreMode;
 import org.junit.jupiter.api.Test;
 import org.opensearch.index.query.BoolQueryBuilder;
 import org.opensearch.index.query.QueryBuilder;
+import org.opensearch.index.query.QueryBuilders;
 import org.opensearch.index.query.WrapperQueryBuilder;
 import org.opensearch.sql.common.setting.Settings;
 import org.opensearch.sql.data.type.ExprCoreType;
@@ -712,9 +714,8 @@ void pushDownFilter_efficient_rejectsScriptSubtree() {
             true,
             rebuildWithFilter);
 
-    // WHERE price + 1 > 100: the outer > is not a direct field ref, so FilterQueryBuilder
-    // falls through to buildScriptQuery() and produces a ScriptQueryBuilder. Under filter_type=
-    // efficient that would be embedded under knn.filter, which AOSS rejects.
+    // price + 1 > 100 lowers to a ScriptQueryBuilder; embedding it under knn.filter would
+    // trigger the AOSS rejection this PR guards against.
     var condition =
         DSL.greater(
             DSL.add(new ReferenceExpression("price", ExprCoreType.INTEGER), DSL.literal(1)),
@@ -734,9 +735,7 @@ void pushDownFilter_efficient_rejectsScriptSubtree() {
 
   @Test
   void pushDownFilter_post_allowsScriptSubtree() {
-    // Under POST mode, script queries are fine: the WHERE lives in an outer bool.filter
-    // that OpenSearch evaluates against hits, not inside knn.filter. Verify we do not
-    // false-positive on the POST branch.
+    // POST puts WHERE in an outer bool.filter, not under knn.filter, so scripts are fine.
     var requestBuilder = createRequestBuilder();
     var knnQuery = new WrapperQueryBuilder("{\"knn\":{}}");
     var builder = new VectorSearchQueryBuilder(requestBuilder, knnQuery, Map.of("k", "5"));
@@ -776,6 +775,78 @@ void buildSucceedsRadialWithSortEmbeddedLimit() {
     assertNotNull(result);
   }
 
+  // ── filter_type=efficient allow-list validator ──────────────────────
+
+  @Test
+  void validateEfficientFilterSafe_rejectsNestedQuery() {
+    // FilterQueryBuilder emits NestedQueryBuilder for SQL nested(field, pred); nested vector
+    // semantics are outside the P0 preview so rejection must be targeted, not generic.
+    QueryBuilder nested =
+        QueryBuilders.nestedQuery(
+            "parent", QueryBuilders.termQuery("parent.f", "v"), ScoreMode.None);
+
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> VectorSearchQueryBuilder.validateEfficientFilterSafe(nested));
+    assertTrue(
+        ex.getMessage().contains("filter_type=efficient does not support nested predicates"),
+        "Expected targeted nested rejection, got: " + ex.getMessage());
+  }
+
+  @Test
+  void validateEfficientFilterSafe_rejectsNestedBuriedInBool() {
+    // AND-ing nested() with a term must still be caught; otherwise the guard is trivially bypassed.
+    QueryBuilder tree =
+        QueryBuilders.boolQuery()
+            .filter(QueryBuilders.termQuery("state", "CA"))
+            .filter(
+                QueryBuilders.nestedQuery(
+                    "parent", QueryBuilders.termQuery("parent.f", "v"), ScoreMode.None));
+
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> VectorSearchQueryBuilder.validateEfficientFilterSafe(tree));
+    assertTrue(ex.getMessage().contains("nested predicates"));
+  }
+
+  @Test
+  void validateEfficientFilterSafe_acceptsBoolOfSafeLeaves() {
+    QueryBuilder tree =
+        QueryBuilders.boolQuery()
+            .filter(QueryBuilders.termQuery("category", "shoes"))
+            .filter(QueryBuilders.rangeQuery("price").gte(80).lte(150));
+
+    VectorSearchQueryBuilder.validateEfficientFilterSafe(tree);
+  }
+
+  @Test
+  void validateEfficientFilterSafe_acceptsExistsLeaf() {
+    // IS NOT NULL lowers to ExistsQueryBuilder; locks in allow-list coverage for that path.
+    QueryBuilder exists = QueryBuilders.existsQuery("brand");
+
+    VectorSearchQueryBuilder.validateEfficientFilterSafe(exists);
+  }
+
+  @Test
+  void validateEfficientFilterSafe_rejectsUnknownWrapper() {
+    // Unknown shapes must fail closed so future FilterQueryBuilder additions cannot silently
+    // re-introduce the AOSS-rejection bug class this PR is guarding against.
+    QueryBuilder unknown = new WrapperQueryBuilder("{\"term\":{\"f\":\"v\"}}");
+
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> VectorSearchQueryBuilder.validateEfficientFilterSafe(unknown));
+    assertTrue(
+        ex.getMessage().contains("unsupported filter query shape"),
+        "Expected unknown-shape rejection, got: " + ex.getMessage());
+    assertTrue(
+        ex.getMessage().contains("WrapperQueryBuilder"),
+        "Expected class name in message, got: " + ex.getMessage());
+  }
+
   private OpenSearchRequestBuilder createRequestBuilder() {
     return new OpenSearchRequestBuilder(
         mock(OpenSearchExprValueFactory.class), 10000, mock(Settings.class));