xiangfu0 commented on code in PR #19303:
URL: https://github.com/apache/pinot/pull/19303#discussion_r3890385352
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/impl/vector/MutableVectorIndexTest.java:
##########
@@ -80,12 +84,180 @@ public void testRuntimeControlDebugInfoReflectsOverrides()
{
Assert.assertEquals(debugInfo.get("effectiveEfSearch"), 6);
Assert.assertEquals(debugInfo.get("effectiveHnswUseRelativeDistance"),
Boolean.FALSE);
Assert.assertEquals(debugInfo.get("effectiveHnswUseBoundedQueue"),
Boolean.FALSE);
- Assert.assertEquals(debugInfo.get("supportsPreFilter"), Boolean.FALSE);
+ Assert.assertEquals(debugInfo.get("supportsPreFilter"), Boolean.TRUE);
+ Assert.assertTrue(index.supportsPreFilter(),
+ "The reader must advertise filtered search: that is what makes the
planner choose it over an exact scan");
} finally {
index.close();
}
}
+ // -----------------------------------------------------------------------
+ // Filtered search (upsert doc-ids snapshot enforcement)
+ // -----------------------------------------------------------------------
+
+ /// 2-D corpus with distinct distances from the query vector {1, 0}:
+ /// docs 0 and 1 are nearest (the "upsert-obsoleted" rows), docs 2 and 3 are
the valid rows.
+ private static MutableVectorIndex create2DIndex(long commitDocs, int
docIdOffset) {
+ Map<String, String> properties = new HashMap<>();
+ properties.put("commitDocs", String.valueOf(commitDocs));
+ properties.put("vectorIndexType", "HNSW");
+ properties.put("vectorDimension", "2");
+ VectorIndexConfig config = new VectorIndexConfig(false, "HNSW", 2, 1,
+ VectorIndexConfig.VectorDistanceFunction.EUCLIDEAN, properties);
+ MutableVectorIndex index =
+ new MutableVectorIndex("mutableVectorIndexFilterTest_" +
System.nanoTime(), COLUMN_NAME, config);
+ addVector(index, new float[]{1.0F, 0.0F}, docIdOffset);
+ addVector(index, new float[]{0.99F, 0.01F}, docIdOffset + 1);
+ addVector(index, new float[]{0.0F, 1.0F}, docIdOffset + 2);
+ addVector(index, new float[]{0.0F, -1.0F}, docIdOffset + 3);
+ return index;
+ }
+
+ @Test
+ public void testFilteredSearchExcludesNearestDisallowedDocs() {
+ // commitDocs=4 commits on the 4th add, so the unfiltered committed-view
sanity check below sees all rows
+ MutableVectorIndex index = create2DIndex(4, 0);
+ try {
+ // Sanity: unfiltered top-2 returns the physically nearest ("obsolete")
docs 0 and 1
+ ImmutableRoaringBitmap unfiltered = index.getDocIds(new float[]{1.0F,
0.0F}, 2);
+ Assert.assertEquals(unfiltered, ImmutableRoaringBitmap.bitmapOf(0, 1));
+
+ // Filtered top-2 restricted to docs 2 and 3 must return exactly those
docs. A post-intersection
+ // implementation would return empty here (the unfiltered top-2 has no
overlap with the allowed set),
+ // so this assertion genuinely discriminates filtered candidate
generation.
+ ImmutableRoaringBitmap filtered =
+ index.getDocIds(new float[]{1.0F, 0.0F}, 2,
ImmutableRoaringBitmap.bitmapOf(2, 3));
+ Assert.assertEquals(filtered, ImmutableRoaringBitmap.bitmapOf(2, 3),
+ "Filtered search must return the allowed docs, not the nearest
disallowed ones");
+ } finally {
+ index.close();
+ }
+ }
+
+ @Test
+ public void testConcurrentFilteredSearchWithLiveWriter()
+ throws Exception {
+ // Single writer, concurrent reader: filtered searches must stay correct
(results always a subset of
+ // the filter bitmap) while rows are being added and commits fire mid-run
+ Map<String, String> properties = new HashMap<>();
+ properties.put("commitDocs", "7");
+ properties.put("vectorIndexType", "HNSW");
+ properties.put("vectorDimension", "2");
+ VectorIndexConfig config = new VectorIndexConfig(false, "HNSW", 2, 1,
+ VectorIndexConfig.VectorDistanceFunction.EUCLIDEAN, properties);
+ MutableVectorIndex index =
+ new MutableVectorIndex("mutableVectorIndexConcurrentTest_" +
System.nanoTime(), COLUMN_NAME, config);
+ int numDocs = 200;
+ ImmutableRoaringBitmap allowed = ImmutableRoaringBitmap.bitmapOf(2, 3);
+ AtomicReference<Throwable> failure =
+ new AtomicReference<>();
+ try {
+ addVector(index, new float[]{0.0F, 1.0F}, 0);
+ addVector(index, new float[]{0.0F, -1.0F}, 1);
+ addVector(index, new float[]{1.0F, 0.0F}, 2);
+ addVector(index, new float[]{0.99F, 0.01F}, 3);
+
+ Thread writer = new Thread(() -> {
+ try {
+ for (int docId = 4; docId < numDocs; docId++) {
+ addVector(index, new float[]{-1.0F, 0.0F}, docId);
+ }
+ } catch (Throwable t) {
+ failure.compareAndSet(null, t);
+ }
+ });
+ writer.start();
+ while (writer.isAlive() && failure.get() == null) {
Review Comment:
Fixed. The writer now waits on a latch for the reader to start and holds
each row until the reader has completed a minimum number of searches, so the
two genuinely overlap instead of the writer racing to completion. The loop
asserts that minimum was met, and the test has `timeOut = 60_000` so a
writer/refresh deadlock -- the bug class it exists to catch -- fails it instead
of hanging the suite.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/vector/IvfPqVectorIndexReader.java:
##########
@@ -217,6 +217,13 @@ public MutableRoaringBitmap getDocIds(float[] searchQuery,
int topK) {
return result;
}
+ @Override
+ public boolean supportsPreFilter() {
Review Comment:
Added
`IvfPqVectorIndexTest#testSupportsPreFilterAndRestrictsResultsToTheFilter`. It
asserts `supportsPreFilter()` is true (so removing the override fails rather
than silently downgrading every IVF_PQ query to an exact scan), that filtered
results are a subset of the supplied bitmap, and that an empty filter yields
nothing. IVF_FLAT and IVF_ON_DISK already had filter-aware coverage; IVF_PQ was
the gap.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/vector/MutableVectorIndex.java:
##########
@@ -152,16 +193,39 @@ public void add(Object[] values, @Nullable int[] dictIds,
int docId) {
@Override
public MutableRoaringBitmap getDocIds(float[] vector, int topK) {
+ return submitSearch(vector, topK, null);
+ }
+
+ @Override
+ public boolean supportsPreFilter() {
+ // Every filtered call restricts its candidates to the supplied bitmap; it
never degrades to an unfiltered
+ // search, so the engine can rely on filtered search instead of an exact
scan.
+ return true;
Review Comment:
Worth checking the diff here: this PR changes the default to `false`, so the
overrides are what keep the in-tree readers doing filtered search.
The reasoning is that `supportsPreFilter()` became a correctness contract in
this PR -- the planner uses it to choose filtered ANN over an exact scan, and a
reader that answers yes without honouring the filter silently drops rows. The
previous Javadoc explicitly allowed conditional filtering, so an out-of-tree
reader written against it would now be asserting a guarantee it never made.
Defaulting to `false` makes that degrade to a slower-but-correct exact scan
instead.
If you would rather keep the default `true` and drop the five overrides,
that is a smaller diff and I am happy to switch -- it just trades out-of-tree
safety for brevity.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
