Skip to content

Commit fa444fe

Browse files
committed
Defer k-NN plugin probe to scan open() so _explain keeps working
The original placement ran knnCapability.requireInstalled() at the very top of applyArguments(). That hook runs during analysis for both _query AND _explain, which regressed two existing contracts on clusters without the k-NN plugin: - VectorSearchExplainIT explicitly states "_explain only parses and plans the query and does NOT require the k-NN plugin". The check would have made every explain test fail with "k-NN plugin missing". - VectorSearchIT is the local-validation/error-path suite. Malformed queries on a pluginless cluster would have collapsed to the k-NN error instead of the existing SQL validation errors. Move the probe to VectorSearchIndexScan.open() — a new thin subclass of OpenSearchIndexScan whose only override calls requireInstalled() before super.open(). Scan open() runs only on the execute path; _explain walks the built plan without opening it, so explain stays functional. Also reorder applyArguments() — nothing to relocate now that the capability check has moved, but the comment documents the intent: local validation runs first so malformed-query errors are stable regardless of cluster state or probe outcome. Add two ITs on the default (pluginless) integ-test cluster: - testExecutionWithoutKnnPluginReturnsCapabilityError — verifies the clear "k-NN plugin not installed" error surfaces on execute. - testExplainWithoutKnnPluginStillWorks — guards the _explain contract so future probe relocations can't silently break it. Also drops the "SQL preview" framing from VectorSearchIndexScanBuilder's aggregation-rejection error per prior user instruction. Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 281beb2 commit fa444fe

8 files changed

Lines changed: 164 additions & 25 deletions

File tree

integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,4 +262,42 @@ public void testBareAggregateRejects() throws IOException {
262262
ex.getMessage(),
263263
containsString("Aggregations are not supported on vectorSearch() relations"));
264264
}
265+
266+
// ── k-NN plugin capability check ──────────────────────────────────────
267+
// The default integ-test cluster does not have the k-NN plugin installed. Execution-path
268+
// queries against vectorSearch() should therefore fail with the clear "k-NN plugin missing"
269+
// error from KnnPluginCapability, while _explain continues to work because the capability
270+
// probe is deferred to scan open() and does not run during analysis/planning.
271+
272+
@Test
273+
public void testExecutionWithoutKnnPluginReturnsCapabilityError() throws IOException {
274+
ResponseException ex =
275+
expectThrows(
276+
ResponseException.class,
277+
() ->
278+
executeQuery(
279+
"SELECT v._id FROM vectorSearch(table='"
280+
+ TEST_INDEX
281+
+ "', field='embedding', vector='[1.0, 2.0]', option='k=5') AS v "
282+
+ "LIMIT 5"));
283+
284+
assertThat(ex.getMessage(), containsString("k-NN plugin"));
285+
assertThat(ex.getMessage(), containsString("not installed"));
286+
}
287+
288+
@Test
289+
public void testExplainWithoutKnnPluginStillWorks() throws IOException {
290+
// _explain only parses and plans the query. It must NOT require the k-NN plugin — the
291+
// capability probe is intentionally deferred to scan open() so pluginless clusters can
292+
// still inspect query plans. If this test starts failing with "k-NN plugin not installed",
293+
// the probe has leaked back into an analysis-time path.
294+
String explain =
295+
explainQuery(
296+
"SELECT v._id FROM vectorSearch(table='"
297+
+ TEST_INDEX
298+
+ "', field='embedding', vector='[1.0, 2.0]', option='k=5') AS v "
299+
+ "LIMIT 5");
300+
301+
assertThat(explain, containsString("wrapper"));
302+
}
265303
}

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/VectorSearchIndex.java

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313
import org.opensearch.sql.common.setting.Settings;
1414
import org.opensearch.sql.opensearch.client.OpenSearchClient;
1515
import org.opensearch.sql.opensearch.request.OpenSearchRequestBuilder;
16+
import org.opensearch.sql.opensearch.storage.capability.KnnPluginCapability;
1617
import org.opensearch.sql.opensearch.storage.scan.OpenSearchIndexScan;
18+
import org.opensearch.sql.opensearch.storage.scan.VectorSearchIndexScan;
1719
import org.opensearch.sql.opensearch.storage.scan.VectorSearchIndexScanBuilder;
1820
import org.opensearch.sql.opensearch.storage.scan.VectorSearchQueryBuilder;
1921
import org.opensearch.sql.storage.read.TableScanBuilder;
@@ -27,6 +29,10 @@ public class VectorSearchIndex extends OpenSearchIndex {
2729
private final float[] vector;
2830
private final Map<String, String> options;
2931
private final FilterType filterType; // null means default (POST)
32+
// Nullable for back-compat with existing tests and the non-vector-search constructor. When
33+
// present, the scan defers a lazy k-NN plugin probe to open() so execution fails fast with a
34+
// clear SQL error if the plugin is missing.
35+
private final KnnPluginCapability knnCapability;
3036

3137
public VectorSearchIndex(
3238
OpenSearchClient client,
@@ -35,12 +41,25 @@ public VectorSearchIndex(
3541
String field,
3642
float[] vector,
3743
Map<String, String> options,
38-
FilterType filterType) {
44+
FilterType filterType,
45+
KnnPluginCapability knnCapability) {
3946
super(client, settings, indexName);
4047
this.field = field;
4148
this.vector = vector;
4249
this.options = options;
4350
this.filterType = filterType;
51+
this.knnCapability = knnCapability;
52+
}
53+
54+
public VectorSearchIndex(
55+
OpenSearchClient client,
56+
Settings settings,
57+
String indexName,
58+
String field,
59+
float[] vector,
60+
Map<String, String> options,
61+
FilterType filterType) {
62+
this(client, settings, indexName, field, vector, options, filterType, null);
4463
}
4564

4665
/** Default constructor — preserves existing call sites; uses no explicit filter type. */
@@ -51,7 +70,7 @@ public VectorSearchIndex(
5170
String field,
5271
float[] vector,
5372
Map<String, String> options) {
54-
this(client, settings, indexName, field, vector, options, null);
73+
this(client, settings, indexName, field, vector, options, null, null);
5574
}
5675

5776
@Override
@@ -89,11 +108,15 @@ public TableScanBuilder createScanBuilder() {
89108
}
90109

91110
Function<OpenSearchRequestBuilder, OpenSearchIndexScan> createScanOperator =
92-
rb ->
93-
new OpenSearchIndexScan(
94-
getClient(),
95-
rb.getMaxResponseSize(),
96-
rb.build(getIndexName(), cursorKeepAlive, getClient(), getFieldTypes().isEmpty()));
111+
rb -> {
112+
var request =
113+
rb.build(getIndexName(), cursorKeepAlive, getClient(), getFieldTypes().isEmpty());
114+
if (knnCapability != null) {
115+
return new VectorSearchIndexScan(
116+
getClient(), rb.getMaxResponseSize(), request, knnCapability);
117+
}
118+
return new OpenSearchIndexScan(getClient(), rb.getMaxResponseSize(), request);
119+
};
97120
return new VectorSearchIndexScanBuilder(queryBuilder, createScanOperator);
98121
}
99122

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionImplementation.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,10 @@ public String toString() {
9393

9494
@Override
9595
public Table applyArguments() {
96-
knnCapability.requireInstalled();
96+
// Local validation runs first so that malformed queries return stable SQL validation errors
97+
// regardless of cluster state. The k-NN plugin presence is checked later, lazily at scan
98+
// open() time, so analysis-time paths (_explain, local validation) stay functional on
99+
// clusters without k-NN.
97100
validateNamedArgs();
98101
String tableName = getArgumentValue(TABLE);
99102
String fieldName = getArgumentValue(FIELD);
@@ -112,7 +115,7 @@ public Table applyArguments() {
112115
}
113116

114117
return new VectorSearchIndex(
115-
client, settings, tableName, fieldName, vector, options, filterType);
118+
client, settings, tableName, fieldName, vector, options, filterType, knnCapability);
116119
}
117120

118121
private float[] parseVector(String vectorLiteral) {

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/capability/KnnPluginCapability.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
* <p>The probe requires a {@link NodeClient}. In REST-client mode (standalone SQL service) the node
2525
* client is absent and the check is skipped — execution-time errors remain the signal there.
2626
*
27-
* <p>The check runs lazily on the first vectorSearch() invocation so cluster boot is unaffected.
27+
* <p>The check runs lazily at scan open() — i.e. only when a vectorSearch() query is actually
28+
* executed — so analysis-time paths like _explain and local argument validation keep working on
29+
* clusters without k-NN.
2830
*/
2931
public class KnnPluginCapability {
3032

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.opensearch.storage.scan;
7+
8+
import org.opensearch.sql.opensearch.client.OpenSearchClient;
9+
import org.opensearch.sql.opensearch.request.OpenSearchRequest;
10+
import org.opensearch.sql.opensearch.storage.capability.KnnPluginCapability;
11+
12+
/**
13+
* OpenSearch scan for vector-search relations. Delegates everything to {@link OpenSearchIndexScan}
14+
* except for {@link #open()}, where it first verifies the k-NN plugin is installed so we fail fast
15+
* with a clear SQL error before the native request would fail deep in execution. The check is
16+
* deferred to open() (not applyArguments() or the scan builder) so that analysis-time paths like
17+
* _explain continue to work on clusters without k-NN.
18+
*/
19+
public class VectorSearchIndexScan extends OpenSearchIndexScan {
20+
21+
private final KnnPluginCapability knnCapability;
22+
23+
public VectorSearchIndexScan(
24+
OpenSearchClient client,
25+
int maxResponseSize,
26+
OpenSearchRequest request,
27+
KnnPluginCapability knnCapability) {
28+
super(client, maxResponseSize, request);
29+
this.knnCapability = knnCapability;
30+
}
31+
32+
@Override
33+
public void open() {
34+
knnCapability.requireInstalled();
35+
super.open();
36+
}
37+
}

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,9 @@
1111
import org.opensearch.sql.planner.logical.LogicalAggregation;
1212

1313
/**
14-
* Scan builder for vector search relations. Rejects aggregation pushdown as a SQL preview
15-
* constraint: aggregations over vectorSearch() relations are not supported in the current preview.
16-
* Native OpenSearch k-NN does support aggregations alongside similarity search; this restriction is
17-
* specific to the SQL surface and may be lifted in a later release.
14+
* Scan builder for vector search relations. Rejects aggregation pushdown: aggregations over
15+
* vectorSearch() relations are not supported in the SQL surface. Native OpenSearch k-NN does
16+
* support aggregations alongside similarity search; this restriction is specific to the SQL layer.
1817
*/
1918
public class VectorSearchIndexScanBuilder extends OpenSearchIndexScanBuilder {
2019

@@ -27,6 +26,6 @@ public VectorSearchIndexScanBuilder(
2726
@Override
2827
public boolean pushDownAggregation(LogicalAggregation aggregation) {
2928
throw new ExpressionEvaluationException(
30-
"Aggregations are not supported on vectorSearch() relations in the SQL preview.");
29+
"Aggregations are not supported on vectorSearch() relations.");
3130
}
3231
}

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionImplementationTest.java

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,11 @@ void testApplyArguments() {
6868
}
6969

7070
@Test
71-
void testApplyArgumentsPropagatesKnnCapabilityFailure() {
72-
// If the capability check fires, applyArguments() must not proceed to build the table.
73-
KnnPluginCapability throwingCapability = org.mockito.Mockito.mock(KnnPluginCapability.class);
74-
org.mockito.Mockito.doThrow(new ExpressionEvaluationException("k-NN plugin not installed"))
75-
.when(throwingCapability)
76-
.requireInstalled();
71+
void testApplyArgumentsDoesNotProbeKnnCapability() {
72+
// Contract: applyArguments() runs during analysis (including _explain) and must NOT invoke
73+
// the k-NN plugin probe. The probe is deferred to scan open() so pluginless clusters can
74+
// still explain and validate vectorSearch() queries locally.
75+
KnnPluginCapability observingCapability = org.mockito.Mockito.mock(KnnPluginCapability.class);
7776
FunctionName functionName = FunctionName.of("vectorsearch");
7877
List<Expression> args =
7978
List.of(
@@ -83,10 +82,9 @@ void testApplyArgumentsPropagatesKnnCapabilityFailure() {
8382
DSL.namedArgument("option", DSL.literal("k=5")));
8483
VectorSearchTableFunctionImplementation impl =
8584
new VectorSearchTableFunctionImplementation(
86-
functionName, args, client, settings, throwingCapability);
87-
ExpressionEvaluationException ex =
88-
assertThrows(ExpressionEvaluationException.class, impl::applyArguments);
89-
assertTrue(ex.getMessage().contains("k-NN plugin"));
85+
functionName, args, client, settings, observingCapability);
86+
impl.applyArguments();
87+
org.mockito.Mockito.verify(observingCapability, org.mockito.Mockito.never()).requireInstalled();
9088
}
9189

9290
@Test
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.opensearch.storage.scan;
7+
8+
import static org.junit.jupiter.api.Assertions.assertThrows;
9+
import static org.junit.jupiter.api.Assertions.assertTrue;
10+
import static org.mockito.Mockito.doThrow;
11+
import static org.mockito.Mockito.mock;
12+
import static org.mockito.Mockito.never;
13+
import static org.mockito.Mockito.verify;
14+
15+
import org.junit.jupiter.api.Test;
16+
import org.opensearch.sql.exception.ExpressionEvaluationException;
17+
import org.opensearch.sql.opensearch.client.OpenSearchClient;
18+
import org.opensearch.sql.opensearch.request.OpenSearchRequest;
19+
import org.opensearch.sql.opensearch.storage.capability.KnnPluginCapability;
20+
21+
class VectorSearchIndexScanTest {
22+
23+
@Test
24+
void openProbesKnnPluginBeforeFetch() {
25+
OpenSearchClient client = mock(OpenSearchClient.class);
26+
OpenSearchRequest request = mock(OpenSearchRequest.class);
27+
KnnPluginCapability capability = mock(KnnPluginCapability.class);
28+
doThrow(new ExpressionEvaluationException("k-NN plugin missing"))
29+
.when(capability)
30+
.requireInstalled();
31+
32+
VectorSearchIndexScan scan = new VectorSearchIndexScan(client, 10, request, capability);
33+
ExpressionEvaluationException ex =
34+
assertThrows(ExpressionEvaluationException.class, scan::open);
35+
assertTrue(ex.getMessage().contains("k-NN plugin"));
36+
// Capability threw, so the underlying client must not have been touched for this scan.
37+
verify(client, never()).search(request);
38+
}
39+
}

0 commit comments

Comments
 (0)