-
Notifications
You must be signed in to change notification settings - Fork 220
[Feature] Add validation hardening, sort/limit restrictions, and integration tests for vectorSearch() #5324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
mengweieric
merged 10 commits into
opensearch-project:feature/vector-search-p0
from
mengweieric:feature/vector-search-p0-hardening
Apr 14, 2026
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
9ff5d2d
Add mutual exclusivity and k range validation
mengweieric 7f8ed14
Add LIMIT > k rejection in top-k vector search mode
mengweieric 943cb89
Add comprehensive test coverage for vector search hardening
mengweieric 767c7f0
Add resolver argument count edge case tests
mengweieric 19f0af3
Add radial size policy, sort restriction, and integration tests
mengweieric 21ee6dc
Fill test coverage gaps for vector search hardening
mengweieric 534b5f4
Preserve sort.getCount() limit pushdown contract in pushDownSort
mengweieric 0e02488
Add compound predicate and radial+WHERE test coverage
mengweieric 1cf35ce
Route pushDownSort count through LIMIT > k validation
mengweieric 4a744d4
Split explain tests into dedicated VectorSearchExplainIT
mengweieric File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
318 changes: 318 additions & 0 deletions
318
integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,318 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.sql; | ||
|
|
||
| import static org.hamcrest.Matchers.containsString; | ||
|
|
||
| import java.io.IOException; | ||
| import org.junit.Test; | ||
| import org.opensearch.client.ResponseException; | ||
| import org.opensearch.sql.legacy.SQLIntegTestCase; | ||
| import org.opensearch.sql.legacy.TestsConstants; | ||
|
|
||
| /** | ||
| * Integration tests for vectorSearch SQL table function. These tests verify DSL push-down shape via | ||
| * _explain and validation error paths. They do NOT require the k-NN plugin since _explain only | ||
| * parses and plans the query without executing it against a knn index. | ||
| */ | ||
| public class VectorSearchIT extends SQLIntegTestCase { | ||
|
|
||
| @Override | ||
| protected void init() throws Exception { | ||
| // _explain needs the index to exist for field resolution. | ||
| loadIndex(Index.ACCOUNT); | ||
| } | ||
|
|
||
| private static final String TEST_INDEX = TestsConstants.TEST_INDEX_ACCOUNT; | ||
|
|
||
| // ── DSL shape verification via _explain ─────────────────────────────── | ||
|
|
||
| @Test | ||
| public void testExplainTopKProducesKnnQuery() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0, 3.0]', option='k=5') AS v " | ||
| + "LIMIT 5"); | ||
|
|
||
| // WrapperQueryBuilder wraps the knn JSON — verify the wrapper is present | ||
| // and track_scores is enabled for score preservation. | ||
| assertTrue("Explain should contain wrapper query:\n" + explain, explain.contains("wrapper")); | ||
| assertTrue( | ||
| "Explain should contain track_scores:\n" + explain, explain.contains("track_scores")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExplainRadialMaxDistanceProducesKnnQuery() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='max_distance=10.5') AS v " | ||
| + "LIMIT 100"); | ||
|
|
||
| assertTrue("Explain should contain wrapper query:\n" + explain, explain.contains("wrapper")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExplainRadialMinScoreProducesKnnQuery() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='min_score=0.8') AS v " | ||
| + "LIMIT 100"); | ||
|
|
||
| assertTrue("Explain should contain wrapper query:\n" + explain, explain.contains("wrapper")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExplainPostFilterProducesBoolQuery() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0, 3.0]', option='k=10') AS v " | ||
| + "WHERE v.state = 'TX' " | ||
| + "LIMIT 10"); | ||
|
|
||
| assertTrue("Explain should contain bool query:\n" + explain, explain.contains("bool")); | ||
| assertTrue( | ||
| "Explain should contain must clause (knn in scoring context):\n" + explain, | ||
| explain.contains("must")); | ||
| assertTrue( | ||
| "Explain should contain filter clause (WHERE in non-scoring context):\n" + explain, | ||
| explain.contains("filter")); | ||
| } | ||
|
|
||
|
ahkcs marked this conversation as resolved.
Outdated
|
||
| // ── Validation error paths ──────────────────────────────────────────── | ||
|
|
||
| @Test | ||
| public void testMutualExclusivityRejectsKAndMaxDistance() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[1.0]', option='k=5,max_distance=10') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("Only one of")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testMutualExclusivityRejectsKAndMinScore() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[1.0]', option='k=5,min_score=0.5') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("Only one of")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testKTooLargeRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[1.0]', option='k=10001') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("k must be between 1 and 10000")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testKZeroRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[1.0]', option='k=0') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("k must be between 1 and 10000")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUnknownOptionKeyRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[1.0]', option='k=5,method.ef_search=100') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("Unknown option key")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEmptyVectorRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[]', option='k=5') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("must not be empty")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testInvalidFieldNameRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', " | ||
| + "field='field\\\"injection', vector='[1.0]', option='k=5') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("Invalid field name")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testMissingRequiredOptionRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='t', field='f', " | ||
| + "vector='[1.0]', option='') AS v")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("Missing required option")); | ||
| } | ||
|
|
||
| // ── Sort restriction validation ───────────────────────────────────────── | ||
|
|
||
| @Test | ||
| public void testOrderByScoreDescExplainSucceeds() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='k=5') AS v " | ||
| + "ORDER BY v._score DESC " | ||
| + "LIMIT 5"); | ||
|
|
||
| assertTrue( | ||
| "Explain should succeed with ORDER BY _score DESC:\n" + explain, | ||
| explain.contains("wrapper")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testOrderByNonScoreFieldRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='k=5') AS v " | ||
| + "ORDER BY v.firstname ASC " | ||
| + "LIMIT 5")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("unsupported sort expression")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testOrderByScoreAscRejects() throws IOException { | ||
| ResponseException ex = | ||
| expectThrows( | ||
| ResponseException.class, | ||
| () -> | ||
| executeQuery( | ||
| "SELECT v._id FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='k=5') AS v " | ||
| + "ORDER BY v._score ASC " | ||
| + "LIMIT 5")); | ||
|
|
||
| assertThat(ex.getMessage(), containsString("_score ASC is not supported")); | ||
| } | ||
|
|
||
| // ── Compound predicate and radial + WHERE ─────────────────────────────── | ||
|
|
||
| @Test | ||
| public void testExplainCompoundPredicateProducesBoolQuery() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0, 3.0]', option='k=10') AS v " | ||
| + "WHERE v.state = 'TX' AND v.age > 30 " | ||
| + "LIMIT 10"); | ||
|
|
||
| assertTrue("Explain should contain bool query:\n" + explain, explain.contains("bool")); | ||
| assertTrue( | ||
| "Explain should contain must clause (knn in scoring context):\n" + explain, | ||
| explain.contains("must")); | ||
| assertTrue( | ||
| "Explain should contain filter clause (compound WHERE in non-scoring context):\n" + explain, | ||
| explain.contains("filter")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExplainRadialWithWhereProducesBoolQuery() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='max_distance=10.5') AS v " | ||
| + "WHERE v.state = 'TX' " | ||
| + "LIMIT 100"); | ||
|
|
||
| assertTrue("Explain should contain bool query:\n" + explain, explain.contains("bool")); | ||
| assertTrue( | ||
| "Explain should contain must clause (knn in scoring context):\n" + explain, | ||
| explain.contains("must")); | ||
| assertTrue( | ||
| "Explain should contain filter clause (WHERE in non-scoring context):\n" + explain, | ||
| explain.contains("filter")); | ||
| } | ||
|
|
||
| // ── LIMIT validation ─────────────────────────────────────────────────── | ||
|
|
||
| @Test | ||
| public void testExplainLimitWithinKSucceeds() throws IOException { | ||
| String explain = | ||
| explainQuery( | ||
| "SELECT v._id, v._score " | ||
| + "FROM vectorSearch(table='" | ||
| + TEST_INDEX | ||
| + "', field='embedding', " | ||
| + "vector='[1.0, 2.0]', option='k=10') AS v " | ||
| + "LIMIT 5"); | ||
|
|
||
| assertTrue("Explain should succeed with LIMIT <= k:\n" + explain, explain.contains("wrapper")); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,9 +197,20 @@ private void validateOptions(Map<String, String> options) { | |
| throw new ExpressionEvaluationException( | ||
| "Missing required option: one of k, max_distance, or min_score"); | ||
| } | ||
| // Mutual exclusivity: exactly one search mode allowed | ||
| int modeCount = (hasK ? 1 : 0) + (hasMaxDistance ? 1 : 0) + (hasMinScore ? 1 : 0); | ||
| if (modeCount > 1) { | ||
| throw new ExpressionEvaluationException( | ||
| "Only one of k, max_distance, or min_score may be specified"); | ||
| } | ||
| // Parse and canonicalize numeric values — closes JSON injection via option values | ||
| if (hasK) { | ||
| parseIntOption(options, "k"); | ||
| int k = Integer.parseInt(options.get("k")); | ||
| if (k < 1 || k > 10000) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| throw new ExpressionEvaluationException( | ||
| String.format("k must be between 1 and 10000, got %d", k)); | ||
| } | ||
| } | ||
| if (hasMaxDistance) { | ||
| parseDoubleOption(options, "max_distance"); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the
Explaintest to a separateExplainITsuite so that we can view the results in a yaml or json file directlyref: CalciteExplainIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done. The _explain cases now live in VectorSearchExplainIT, and VectorSearchIT keeps only behavior/validation tests. I kept the current SQL explain assertions for now; happy to follow up with file-backed expected outputs if we want to standardize that too.