Skip to content

Commit 76d9055

Browse files
committed
Address review feedback: reword user-facing error, strengthen explain test, rename constructor comment
- Reword filter_type error message to be user-friendly and actionable (no longer leaks internal ScriptQueryUnSupportedException text) - Strengthen efficient-mode explain IT: assert no bool/must (proves not post-filter shape), decode base64 knn payload to verify filter and predicate field are embedded inside knn query - Rename "Backward-compatible constructor" to clarify intent Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 1db11ce commit 76d9055

4 files changed

Lines changed: 32 additions & 7 deletions

File tree

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

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,33 @@ public void testExplainFilterTypeEfficientProducesKnnWithFilter() throws IOExcep
202202
+ "WHERE v.state = 'TX' "
203203
+ "LIMIT 5");
204204

205-
// Efficient mode: knn rebuilt with filter inside, wrapped in WrapperQueryBuilder
205+
// Efficient mode: knn rebuilt with filter inside, wrapped in WrapperQueryBuilder.
206+
// The knn JSON (including the embedded filter) is base64-encoded inside the wrapper,
207+
// so we verify structure by: (1) wrapper present, (2) no bool/must in plaintext
208+
// (that would be post-filter shape), (3) decode the base64 payload to confirm
209+
// the filter and predicate field are embedded inside the knn query.
206210
assertTrue("Explain should contain wrapper query:\n" + explain, explain.contains("wrapper"));
211+
assertFalse(
212+
"Efficient mode should not produce bool query (that is post-filter shape):\n" + explain,
213+
explain.contains("\"bool\""));
214+
assertFalse(
215+
"Efficient mode should not contain must clause:\n" + explain, explain.contains("\"must\""));
216+
217+
// Extract and decode the base64 knn payload to verify filter embedding.
218+
// The explain output escapes quotes as \", so match both \" and " forms.
219+
java.util.regex.Matcher m =
220+
java.util.regex.Pattern.compile("\\\\?\"query\\\\?\":\\\\?\"([A-Za-z0-9+/=]+)\\\\?\"")
221+
.matcher(explain);
222+
assertTrue("Explain should contain base64-encoded knn query:\n" + explain, m.find());
223+
String knnJson =
224+
new String(
225+
java.util.Base64.getDecoder().decode(m.group(1)),
226+
java.nio.charset.StandardCharsets.UTF_8);
227+
assertTrue(
228+
"Efficient mode knn JSON should contain filter:\n" + knnJson, knnJson.contains("filter"));
229+
assertTrue(
230+
"Efficient mode knn JSON should contain the WHERE predicate field:\n" + knnJson,
231+
knnJson.contains("state"));
207232
}
208233

209234
@Test

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public VectorSearchIndex(
4343
this.filterType = filterType;
4444
}
4545

46-
/** Backward-compatible constructor — defaults to no explicit filter type. */
46+
/** Default constructor — preserves existing call sites; uses no explicit filter type. */
4747
public VectorSearchIndex(
4848
OpenSearchClient client,
4949
Settings settings,

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,8 @@ public boolean pushDownFilter(LogicalFilter filter) {
8484
} catch (ScriptQueryUnSupportedException e) {
8585
if (filterTypeExplicit) {
8686
throw new ExpressionEvaluationException(
87-
"filter_type requires a pushdownable WHERE clause, but the given condition"
88-
+ " cannot be pushed down: "
89-
+ e.getMessage());
87+
"filter_type only works when the WHERE clause can be translated to an"
88+
+ " OpenSearch filter. Rewrite the WHERE clause or omit filter_type.");
9089
}
9190
// Default mode: fall back to in-memory filtering (matches base class behavior)
9291
return false;

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -571,8 +571,9 @@ void pushDownFilterNonPushdownableWithExplicitFilterTypeThrows() {
571571

572572
ExpressionEvaluationException ex =
573573
assertThrows(ExpressionEvaluationException.class, () -> builder.pushDownFilter(filter));
574-
assertTrue(ex.getMessage().contains("filter_type requires a pushdownable WHERE clause"));
575-
assertTrue(ex.getMessage().contains("cannot be pushed down"));
574+
assertTrue(
575+
ex.getMessage().contains("filter_type only works when the WHERE clause can be translated"));
576+
assertTrue(ex.getMessage().contains("Rewrite the WHERE clause or omit filter_type"));
576577
}
577578

578579
@Test

0 commit comments

Comments
 (0)