xiangfu0 commented on code in PR #19303: URL: https://github.com/apache/pinot/pull/19303#discussion_r3890384215
########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/vector/BaseDocIdBitmapFilterQuery.java: ########## @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.local.segment.index.readers.vector; + +import java.io.IOException; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.ConstantScoreWeight; +import org.apache.lucene.search.DocIdSetIterator; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.QueryVisitor; +import org.apache.lucene.search.ScoreMode; +import org.apache.lucene.search.Scorer; +import org.apache.lucene.search.Weight; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; + + +/// Base class for Lucene [Query] implementations that accept only documents whose PINOT doc id is present +/// in a [ImmutableRoaringBitmap]. Used to implement pre-filter ANN search by restricting HNSW graph +/// traversal to the filtered document set. +/// +/// Because Lucene uses its own internal doc ids (which differ from Pinot doc ids), subclasses supply the +/// per-leaf iterator that maps Lucene doc ids to Pinot doc ids before testing membership in the bitmap +/// (via a doc-id translator, doc values, etc.). This class owns the constant-score weight/scorer +/// scaffolding, identity-based equality, and cache opt-out, so filter-correctness fixes apply to every +/// implementation at once. +/// +/// Instances are single-use per search and must never be cached by Lucene ([Weight#isCacheable] returns +/// false), since the accepted docs depend on the bitmap instance. +public abstract class BasePinotDocIdBitmapFilterQuery extends Query { + protected final ImmutableRoaringBitmap _bitmap; Review Comment: Sorry, I misread the first time. Done as suggested: the class is `BaseFilterQuery` and the field is `_docIds`. _🤖 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: ########## @@ -230,33 +294,140 @@ public Map<String, Object> getIndexDebugInfo() { info.put("effectiveEfSearch", getEffectiveEfSearch()); info.put("effectiveHnswUseRelativeDistance", getEffectiveUseRelativeDistance()); info.put("effectiveHnswUseBoundedQueue", getEffectiveUseBoundedQueue()); - info.put("supportsPreFilter", false); + info.put("supportsPreFilter", true); try (DirectoryReader directoryReader = DirectoryReader.open(_indexDirectory)) { info.put("numDocs", directoryReader.numDocs()); info.put("numDeletedDocs", directoryReader.numDeletedDocs()); info.put("luceneSegments", directoryReader.leaves().size()); } catch (IOException e) { LOGGER.warn("Failed to load mutable HNSW debug stats for segment: {}, column: {}", _segmentName, _vectorColumn, e); - info.put("numDocs", _nextDocId); + info.put("numDocs", _numDocsAdded); info.put("numDeletedDocs", 0); info.put("luceneSegments", 0); } return info; } private MutableRoaringBitmap executeVectorSearch(float[] vector, int topK, int efSearch, - boolean useRelativeDistance, boolean useBoundedQueue) throws IOException { + boolean useRelativeDistance, boolean useBoundedQueue, @Nullable ImmutableRoaringBitmap preFilterBitmap) + throws IOException { + if (preFilterBitmap != null) { + // Filtered search enforces the query's visible-document set, so it must see every row that set names -- + // including rows still in the writer's RAM buffer. Refreshing is expensive: maybeRefreshBlocking takes an + // exclusive lock (it does not coalesce; that is maybeRefresh), and on an actively consuming segment the + // reopen always flushes the writer, which stalls indexing. So only refresh when this query can actually + // see past the last refresh. The added-doc watermark is read BEFORE refreshing so rows arriving during + // the refresh are not wrongly claimed as visible. + if (!preFilterBitmap.isEmpty() && preFilterBitmap.last() > _searcherRefreshedThroughDocId) { Review Comment: You are right, and thank you for catching it -- I had quoted the `MutableIndex#add` contract about arbitrary doc-id order earlier in this change and then built a watermark that assumed the opposite. Now tracking the writer sequence number returned by `IndexWriter#addDocument` instead of the maximum doc id, which is monotonic by construction regardless of doc-id order. Added `testFilteredSearchSeesOutOfOrderUncommittedDoc`, which reproduces your exact scenario (refresh through doc 10, add doc 5, search bitmap {5}); I confirmed it fails against a doc-id watermark and passes with the sequence number. On coalescing: the refresh is now skipped entirely whenever nothing has been added since the last one, but you are right that an actively ingesting segment still refreshes per query. `ControlledRealTimeReopenThread#waitForGeneration` is the proper fix and I did not take it here because it adds a thread per index instance, which is significant at Pinot's consuming-segment counts. Happy to do it in this PR if you prefer, otherwise I would rather size it as follow-up work with a concurrent ingest/query benchmark. _🤖 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]
