[BugFix] Reject OFFSET, WHERE on _score, and script subtrees under filter_type=efficient on vectorSearch()#5374
Conversation
mengweieric
requested review from
LantaoJin,
RyanL1997,
Swiddis,
acarbonetto,
ahkcs,
anirudha,
dai-chen,
joshuali925,
noCharger,
penghuo,
ps48,
qianheng-aws,
songkant-aws,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit c723515. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
mengweieric
added
bug
mengweieric
force-pushed
the
feature/vector-search-p0
branch
from
9aa62fc to
fa444fe
Compare
mengweieric
force-pushed
the
fix/vector-search-query-builder-hardening
branch
3 times, most recently
from
55160f9 to
3945b72
Compare
…=efficient Three bug fixes in VectorSearchQueryBuilder that close silent-data-loss paths on vectorSearch() relations: 1. OFFSET on vectorSearch(): pushDownLimit now rejects any LogicalLimit with a non-zero offset. The parent path would push `from: <offset>` into the OpenSearch request, silently shifting the top-k window and dropping the highest-scoring results. LogicalSort carries only a count (no offset), so no change is needed on the sort path. 2. WHERE on _score: pushDownFilter walks the condition with an ExpressionNodeVisitor before compiling to DSL and rejects any ReferenceExpression whose attr matches "_score" case-insensitively. Previously the synthetic _score column compiled to a range query on a non-existent stored field, which returned zero rows at the cluster with no error. Users who want a score floor should use option='min_score=...'. 3. Script subtrees under filter_type=efficient: after building the WHERE QueryBuilder, the EFFICIENT branch recursively scans for any ScriptQueryBuilder (traversing BoolQueryBuilder's must/filter/should/ mustNot lists and ConstantScoreQueryBuilder wrappers). If found, refuse to embed it under knn.filter. AOSS and serverless vector collections reject script queries inside knn.filter; without this guard the user saw an opaque cluster-side failure instead of a clear plan-time error. Unit tests cover each rejection (including the compound predicate path for _score, the uppercase _SCORE bypass attempt, and the POST-mode negative case for script predicates). Integration tests assert 400 status and the user-facing message from the SQL endpoint, including the combined sort + limit + offset shape that exercises the pushDownSort() path. Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
mengweieric
force-pushed
the
fix/vector-search-query-builder-hardening
branch
from
3945b72 to
154df52
Compare
| if (qb instanceof ConstantScoreQueryBuilder) { | ||
| return containsScriptQuery(((ConstantScoreQueryBuilder) qb).innerQuery()); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
The walker handles BoolQueryBuilder and ConstantScoreQueryBuilder, and falls through to return false for any other wrapper type. This is correct today because FilterQueryBuilder (the only source of whereQuery here) only produces those shapes plus leaf queries and ScriptQueryBuilder.
The concern: if FilterQueryBuilder later adds a new wrapper type (e.g. NestedQueryBuilder, FunctionScoreQueryBuilder, DisMaxQueryBuilder), a ScriptQueryBuilder nested inside would silently slip past this guard and be embedded under knn.filter, producing exactly the AOSS failure this PR is trying to prevent.
Two options to make this future-proof:
- Fail closed on unknown wrapper types —
throw new ExpressionEvaluationException("unexpected QueryBuilder type " + qb.getClass().getSimpleName() + " in filter_type=efficient path; refusing to embed")instead ofreturn false. Verbose, but safe. - Leave the
return falsebut add a unit test onFilterQueryBuilderthat locks in "all shapes produced by build() must be leaf | BoolQuery | ConstantScoreQuery | ScriptQuery" so future additions fail loudly in tests.
Either is fine — just noting that the current shape carries a "quietly re-introduces the bug" risk if FilterQueryBuilder evolves.
There was a problem hiding this comment.
Agreed, updated. Efficient mode now uses a fail-closed validator instead of a best-effort script walker: scripts are rejected, nested
predicates are rejected for the P0 preview, known safe leaf query builders are allowed, known containers are recursively checked, and unknown
query shapes fail closed. Added tests for nested rejection, safe leaf trees, exists, and unknown wrappers.
| // 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. | ||
| if (limit.getOffset() != null && limit.getOffset() != 0) { |
There was a problem hiding this comment.
Minor: limit.getOffset() — is this Integer (boxed) or int (primitive)? If LogicalLimit.getOffset() returns primitive int, the != null check is always true and dead. Worth verifying: if boxed, keep the null-check for defensive coding; if primitive, drop it for readability.
…ent 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>
mengweieric
merged commit 01a5d87
into
opensearch-project:feature/vector-search-p0
2 participants
Summary
_scorecolumn. Previously these pushed a range query on a non-existent field and silently returned 0 rows. Users should useoption='min_score=...'instead.filter_type=efficient. Previously WHERE predicates that compile to script queries (arithmetic, function calls, CASE, date math) were embedded underknn.filter, which is incompatible with AOSS/serverless vector collections.Test plan