xiangfu0 commented on code in PR #19297:
URL: https://github.com/apache/pinot/pull/19297#discussion_r3865278773
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/ExactVectorScanFilterOperator.java:
##########
@@ -74,28 +79,21 @@ public class ExactVectorScanFilterOperator extends
BaseFilterOperator {
/// @param predicate the vector similarity predicate containing query vector
and top-K
/// @param column the column name (for logging and explain)
/// @param numDocs the total number of documents in the segment
- public ExactVectorScanFilterOperator(ForwardIndexReader<?>
forwardIndexReader,
- VectorSimilarityPredicate predicate, String column, int numDocs) {
- this(forwardIndexReader, predicate, column, numDocs, null,
"vector_index_missing",
- VectorSearchParams.DEFAULT);
- }
-
+ /// @param vectorIndexConfig vector index configuration, used to resolve the
distance function (may be null)
+ /// @param fallbackReason why this scan runs instead of an ANN search,
reported in explain output
+ /// @param searchParams vector search parameters from query options
+ /// @param candidateScope documents this scan may consider, or null to scan
the whole segment
public ExactVectorScanFilterOperator(ForwardIndexReader<?>
forwardIndexReader,
VectorSimilarityPredicate predicate, String column, int numDocs,
@Nullable VectorIndexConfig vectorIndexConfig,
- String fallbackReason) {
- this(forwardIndexReader, predicate, column, numDocs, vectorIndexConfig,
fallbackReason,
- VectorSearchParams.DEFAULT);
- }
-
- public ExactVectorScanFilterOperator(ForwardIndexReader<?>
forwardIndexReader,
- VectorSimilarityPredicate predicate, String column, int numDocs,
@Nullable VectorIndexConfig vectorIndexConfig,
- String fallbackReason, VectorSearchParams searchParams) {
+ String fallbackReason, VectorSearchParams searchParams,
+ @Nullable VectorCandidateScope candidateScope) {
Review Comment:
Reformatted — the parameter list now packs to the line limit instead of
breaking early.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/VectorCandidateScope.java:
##########
@@ -0,0 +1,66 @@
+/**
+ * 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.core.operator.filter;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+/// Immutable set of document IDs a vector predicate is allowed to consider as
candidates.
+///
+/// Vector top-K is not monotonic: unlike an ordinary predicate, restricting
the corpus changes which documents win
+/// the top-K contest, so a document set that defines what the query may see
must be applied *before* candidate
+/// generation rather than intersected with the result afterwards. That
contract is the entire meaning of this type.
+/// It deliberately says nothing about where the restriction came from --
callers such as [FilterPlanNode] decide
+/// that, today from the segment's queryable-document snapshot.
+///
+/// Contrast with the optimizer-selected metadata bitmap passed to
+/// [VectorSimilarityFilterOperator#setPreFilterBitmap]: that one is optional,
because post-filtering an ordinary
+/// predicate is already correct and pre-filtering only improves recall.
+///
+/// The factory owns the single detached copy of the supplied bitmap, so
operators can share a scope freely and only
+/// allocate again when they intersect it with an independent optional filter.
+public final class VectorCandidateScope {
Review Comment:
Agreed — removed the class and pass the bitmap directly.
To answer the question: there was a planned extension (a follow-up makes the
mutable HNSW index filter-aware, which needs the candidate set bounded by the
published-document watermark as well). But in this PR the type holds exactly
one bitmap, so it is speculative generality here. If that second field actually
materializes, introducing a type then will be a smaller and better-justified
change than keeping an empty wrapper now.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/VectorCandidateScope.java:
##########
@@ -0,0 +1,66 @@
+/**
+ * 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.core.operator.filter;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+/// Immutable set of document IDs a vector predicate is allowed to consider as
candidates.
+///
+/// Vector top-K is not monotonic: unlike an ordinary predicate, restricting
the corpus changes which documents win
+/// the top-K contest, so a document set that defines what the query may see
must be applied *before* candidate
+/// generation rather than intersected with the result afterwards. That
contract is the entire meaning of this type.
+/// It deliberately says nothing about where the restriction came from --
callers such as [FilterPlanNode] decide
+/// that, today from the segment's queryable-document snapshot.
+///
+/// Contrast with the optimizer-selected metadata bitmap passed to
+/// [VectorSimilarityFilterOperator#setPreFilterBitmap]: that one is optional,
because post-filtering an ordinary
+/// predicate is already correct and pre-filtering only improves recall.
+///
+/// The factory owns the single detached copy of the supplied bitmap, so
operators can share a scope freely and only
+/// allocate again when they intersect it with an independent optional filter.
+public final class VectorCandidateScope {
+ private final ImmutableRoaringBitmap _requiredDocIds;
+
+ private VectorCandidateScope(ImmutableRoaringBitmap requiredDocIds) {
+ // Detach from the caller's bitmap so candidate generation and the outer
valid-document AND observe the same
+ // document set for the whole query. Copying into array-backed containers
keeps per-document `contains` checks
+ // cheap, which matters because filtered graph traversal probes this
bitmap once per visited node.
+ _requiredDocIds =
requiredDocIds.toMutableRoaringBitmap().toImmutableRoaringBitmap();
+ }
+
+ /// Creates a scope from the document IDs the query is allowed to consider.
+ public static VectorCandidateScope of(ImmutableRoaringBitmap requiredDocIds)
{
Review Comment:
Moot now — the class is gone entirely and the bitmap is passed directly, so
there is no factory or constructor to choose between.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/VectorCandidateScope.java:
##########
@@ -0,0 +1,66 @@
+/**
+ * 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.core.operator.filter;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+/// Immutable set of document IDs a vector predicate is allowed to consider as
candidates.
+///
+/// Vector top-K is not monotonic: unlike an ordinary predicate, restricting
the corpus changes which documents win
+/// the top-K contest, so a document set that defines what the query may see
must be applied *before* candidate
+/// generation rather than intersected with the result afterwards. That
contract is the entire meaning of this type.
+/// It deliberately says nothing about where the restriction came from --
callers such as [FilterPlanNode] decide
+/// that, today from the segment's queryable-document snapshot.
+///
+/// Contrast with the optimizer-selected metadata bitmap passed to
+/// [VectorSimilarityFilterOperator#setPreFilterBitmap]: that one is optional,
because post-filtering an ordinary
+/// predicate is already correct and pre-filtering only improves recall.
+///
+/// The factory owns the single detached copy of the supplied bitmap, so
operators can share a scope freely and only
+/// allocate again when they intersect it with an independent optional filter.
+public final class VectorCandidateScope {
+ private final ImmutableRoaringBitmap _requiredDocIds;
+
+ private VectorCandidateScope(ImmutableRoaringBitmap requiredDocIds) {
+ // Detach from the caller's bitmap so candidate generation and the outer
valid-document AND observe the same
+ // document set for the whole query. Copying into array-backed containers
keeps per-document `contains` checks
+ // cheap, which matters because filtered graph traversal probes this
bitmap once per visited node.
+ _requiredDocIds =
requiredDocIds.toMutableRoaringBitmap().toImmutableRoaringBitmap();
Review Comment:
Removed, along with the copy it was describing.
The comment was trying to justify defensively copying the caller bitmap.
That was not worth doing: master already hands the same valid-document snapshot
straight to BitmapBasedFilterOperator and iterates it during execution, so
copying here bought no safety that the surrounding code does not already
assume, and cost an allocation per segment per query. The operator now holds
the bitmap by reference with a documented do-not-modify contract, matching that
existing convention.
_🤖 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]
