xiangfu0 commented on code in PR #19303:
URL: https://github.com/apache/pinot/pull/19303#discussion_r3890384694
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/vector/MutableVectorIndex.java:
##########
@@ -90,13 +127,11 @@ public MutableVectorIndex(String segmentName, String
vectorColumn, VectorIndexCo
_commitDocs = Long.parseLong(
vectorIndexConfig.getProperties().getOrDefault("commitDocs",
String.valueOf(DEFAULT_COMMIT_DOCS)));
_vectorSimilarityFunction =
VectorIndexUtils.toSimilarityFunction(vectorIndexConfig.getVectorDistanceFunction());
- // Each column of a segment gets its own directory, so that cleaning up
one column does not remove the index of
- // another column of the same segment.
- _indexDir = new File(new File(FileUtils.getTempDirectory(), segmentName),
- _vectorColumn +
V1Constants.Indexes.VECTOR_V912_HNSW_INDEX_FILE_EXTENSION);
+ _indexDir = createIndexDir(segmentName, _vectorColumn);
Review Comment:
Agreed, removed. The directory handling is back to master's
per-segment/per-column layout, and the isolation test is gone with it. I will
send the write-lock fix as its own PR.
_🤖 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:
##########
@@ -279,18 +450,35 @@ public void close() {
try {
_indexWriter.commit();
// IndexWriter does not close the Directory passed to it, so both need
to be closed.
- IOUtils.close(_indexWriter, _indexDirectory);
+ IOUtils.close(_searcherManager, _indexWriter, _indexDirectory);
} catch (IOException e) {
- // Both close() implementations are idempotent, so this is a no-op for
whatever was already closed above.
- IOUtils.closeWhileHandlingException(_indexWriter, _indexDirectory);
+ // All close() implementations are idempotent, so this is a no-op for
whatever was already closed above.
+ IOUtils.closeWhileHandlingException(_searcherManager, _indexWriter,
_indexDirectory);
throw new RuntimeException(e);
} finally {
deleteIndexDir();
}
}
- /// Deletes the temporary index directory of this column, then the segment
directory holding it if this was the last
- /// column with an index under it.
+ /// Creates a private directory for this index instance, under a directory
named for the segment.
+ ///
+ /// The directory is unique per instance rather than per segment and column,
so two replicas of the same segment
+ /// hosted in one JVM cannot collide on the Lucene write lock. Segment and
column stay in the path so a leaked
+ /// directory can still be attributed to its owner.
+ private static File createIndexDir(String segmentName, String column) {
+ File segmentDir = new File(FileUtils.getTempDirectory(), segmentName);
+ try {
+ Files.createDirectories(segmentDir.toPath());
+ return Files.createTempDirectory(segmentDir.toPath(),
+ column + V1Constants.Indexes.VECTOR_V912_HNSW_INDEX_FILE_EXTENSION +
'_').toFile();
+ } catch (IOException e) {
+ throw new RuntimeException(
+ "Failed to create mutable vector index directory for column: " +
column + ", segment: " + segmentName, e);
+ }
+ }
+
+ /// Deletes the temporary index directory of this instance, then the segment
directory holding it if this was the
Review Comment:
Moot now: the per-instance directory change is out of this PR, so
`deleteIndexDir` is back to master's behaviour. The parent-deletion race you
describe is real though, and I will carry it into the separate
directory-isolation PR rather than lose it.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/vector/BaseDocIdBitmapFilterQuery.java:
##########
@@ -0,0 +1,136 @@
+/**
+ * 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 javax.annotation.Nullable;
+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 BaseDocIdBitmapFilterQuery extends Query {
Review Comment:
Documented on `BaseFilterQuery`. It now states that the bitmap is retained
by reference and not copied, that `ImmutableRoaringBitmap` only promises this
type exposes no mutators (a caller may pass a `MutableRoaringBitmap` subtype),
that mutating it mid-search changes which documents are accepted partway
through traversal and yields results matching neither set, and that callers who
cannot honour that must pass a detached copy. Also notes instances are safe to
share across a single search's threads, since the bitmap is only read and each
leaf gets its own iterator.
_🤖 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]
