Copilot commented on code in PR #19297:
URL: https://github.com/apache/pinot/pull/19297#discussion_r3809288724
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/ExactVectorScanFilterOperator.java:
##########
@@ -169,87 +184,113 @@ protected void explainAttributes(ExplainAttributeBuilder
attributeBuilder) {
attributeBuilder.putString("vectorLiteral",
Arrays.toString(_predicate.getValue()));
attributeBuilder.putString("fallbackReason",
_vectorExplainContext.getFallbackReason());
attributeBuilder.putLongIdempotent("topKtoSearch", _predicate.getTopK());
+ attributeBuilder.putBool("upsertCandidateFilterApplied",
_requiredUpsertCandidateBitmap != null);
+ attributeBuilder.putLongIdempotent("upsertCandidateFilterCardinality",
getRequiredUpsertCandidateCardinality());
+ attributeBuilder.putLongIdempotent("effectiveAllowedDocIdsCardinality",
+ getEffectiveAllowedDocIdsCardinality());
Review Comment:
These cardinalities vary per segment, so marking them `IDEMPOTENT` prevents
`PlanNodeMerger` from merging exact-scan nodes whenever valid-document counts
differ; `IDEMPOTENT` values merge only when equal
(`PlanNodeMerger.java:74-77`). Runtime candidate counts use additive `putLong`
instead (`VectorSimilarityFilterOperator.java:286,289`). Emit additive
cardinalities only when a candidate scope exists, otherwise large explains can
expand to one node per segment.
This issue also appears on line 292 of the same file.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/ExactVectorScanFilterOperator.java:
##########
@@ -169,87 +184,113 @@ protected void explainAttributes(ExplainAttributeBuilder
attributeBuilder) {
attributeBuilder.putString("vectorLiteral",
Arrays.toString(_predicate.getValue()));
attributeBuilder.putString("fallbackReason",
_vectorExplainContext.getFallbackReason());
attributeBuilder.putLongIdempotent("topKtoSearch", _predicate.getTopK());
+ attributeBuilder.putBool("upsertCandidateFilterApplied",
_requiredUpsertCandidateBitmap != null);
+ attributeBuilder.putLongIdempotent("upsertCandidateFilterCardinality",
getRequiredUpsertCandidateCardinality());
+ attributeBuilder.putLongIdempotent("effectiveAllowedDocIdsCardinality",
+ getEffectiveAllowedDocIdsCardinality());
}
/// Performs brute-force exact search over all documents in the segment.
/// When a distance threshold is set, returns all vectors within the
threshold.
/// Otherwise uses a max-heap to maintain the top-K closest vectors.
- @SuppressWarnings("unchecked")
private ImmutableRoaringBitmap computeExactTopK() {
+ ImmutableRoaringBitmap allowedDocIds = _requiredUpsertCandidateBitmap;
+ if (allowedDocIds != null && allowedDocIds.isEmpty()) {
+ return new MutableRoaringBitmap();
+ }
LOGGER.warn("Performing exact vector scan fallback on column: {} for
segment with {} docs. "
- + "reason={}, distanceFunction={}, hasThreshold={}. "
+ + "reason={}, distanceFunction={}, hasThreshold={},
allowedDocs={}. "
+ "This is expensive -- consider adding a vector index.",
_column, _numDocs, _vectorExplainContext.getFallbackReason(),
- _vectorExplainContext.getDistanceFunction(), _hasDistanceThreshold);
+ _vectorExplainContext.getDistanceFunction(), _hasDistanceThreshold,
+ allowedDocIds != null ? allowedDocIds.getCardinality() : _numDocs);
float[] queryVector = _predicate.getValue();
+ Float threshold = _hasDistanceThreshold ? _distanceThreshold : null;
+ ImmutableRoaringBitmap result = computeExactMatches(_forwardIndexReader,
queryVector, _predicate.getTopK(),
+ _numDocs, _vectorExplainContext.getDistanceFunction(), threshold,
allowedDocIds, _column);
- if (_hasDistanceThreshold) {
- return computeExactThreshold(queryVector);
- }
+ LOGGER.debug("Exact vector scan on column: {} returned {} results from {}
docs",
+ _column, result.getCardinality(), _numDocs);
- int topK = _predicate.getTopK();
+ return result;
+ }
- // Max-heap: entry with largest distance is at the top so we can
efficiently evict it
- PriorityQueue<DocDistance> maxHeap = new PriorityQueue<>(topK + 1,
- (a, b) -> Float.compare(b._distance, a._distance));
+ /// Performs an exact top-K or threshold search over all documents or the
supplied allowed-document bitmap.
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ static ImmutableRoaringBitmap computeExactMatches(ForwardIndexReader<?>
forwardIndexReader,
+ float[] queryVector, int topK, int numDocs,
+ VectorIndexConfig.VectorDistanceFunction distanceFunction, @Nullable
Float distanceThreshold,
+ @Nullable ImmutableRoaringBitmap allowedDocIds, String column) {
+ if ((allowedDocIds != null && allowedDocIds.isEmpty()) ||
(distanceThreshold == null && topK <= 0)) {
+ return new MutableRoaringBitmap();
Review Comment:
Non-positive `topK` is now silently converted to an empty result, while
indexed backends reject it (for example, `IvfFlatVectorIndexReader.java:212`
and `IvfPqVectorIndexReader.java:164`). The same `VECTOR_SIMILARITY` query can
therefore fail or return no rows depending on whether a segment uses exact
fallback. In top-K mode, validate `topK > 0` before the empty-bitmap shortcut
and update the new test to expect that validation error; threshold mode can
continue to ignore `topK`.
--
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]
