Copilot commented on code in PR #19287:
URL: https://github.com/apache/pinot/pull/19287#discussion_r3801830487
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/VectorSimilarityFilterOperator.java:
##########
@@ -292,31 +381,80 @@ protected void explainAttributes(ExplainAttributeBuilder
attributeBuilder) {
if (explainContext.getFilterSelectivity() >= 0) {
attributeBuilder.putString("filterSelectivity", String.format("%.4f",
explainContext.getFilterSelectivity()));
}
+ attributeBuilder.putBool("upsertCandidateFilterApplied",
_requiredUpsertCandidateBitmap != null);
+ attributeBuilder.putLongIdempotent("upsertCandidateFilterCardinality",
getRequiredUpsertCandidateCardinality());
+ attributeBuilder.putBool("publishedDocRangeApplied",
_publishedDocIdsBitmap != null);
+ attributeBuilder.putLongIdempotent("publishedDocRangeCardinality",
getPublishedDocIdsCardinality());
+ attributeBuilder.putBool("requiredMetadataCandidateFilterApplied",
_requiredMetadataCandidateBitmap != null);
+
attributeBuilder.putLongIdempotent("requiredMetadataCandidateFilterCardinality",
+ getRequiredMetadataCandidateCardinality());
+ attributeBuilder.putLongIdempotent("effectiveAllowedDocIdsCardinality",
+ getEffectiveAllowedDocIdsCardinality());
+ attributeBuilder.putBool("candidateGenerationSkipped",
_candidateGenerationSkipped);
+ if (_candidateGenerationSkipped) {
+ attributeBuilder.putString("candidateGenerationSkipReason",
getCandidateGenerationSkipReason());
+ }
+ if (_runtimeFallbackReason != null) {
+ attributeBuilder.putString("fallbackReason", _runtimeFallbackReason);
+ }
}
/// Returns true if the underlying vector index reader supports pre-filter
ANN search.
public boolean supportsPreFilter() {
- return _vectorIndexReader instanceof FilterAwareVectorIndexReader
- && ((FilterAwareVectorIndexReader)
_vectorIndexReader).supportsPreFilter();
+ return readerSupportsPreFilter();
}
/// Executes the vector search with backend-specific parameter dispatch and
optional rerank.
private ImmutableRoaringBitmap executeSearch() {
String column = _predicate.getLhs().getIdentifier();
float[] queryVector = _predicate.getValue();
VectorExplainContext explainContext = _vectorExplainContext;
+ boolean backendParamsConfigured = false;
+ boolean searchExecuted = false;
try {
+ ImmutableRoaringBitmap effectiveAllowedDocIds = _effectiveAllowedDocIds;
+ if (hasMandatoryCandidateScope() && effectiveAllowedDocIds.isEmpty()) {
+ _candidateGenerationSkipped = true;
+ _annCandidateCount = 0;
+ _rerankedCandidateCount = 0;
+ return new MutableRoaringBitmap();
+ }
+ _candidateGenerationSkipped = false;
+
+ if (hasMandatoryCandidateScope() && !readerSupportsPreFilter()) {
+ if (_forwardIndexReader == null) {
+ throw new IllegalStateException("Cannot honor mandatory vector
candidate scope on vector column: "
+ + column + " -- vector index reader does not support filtered
search and no forward index is available");
+ }
+ _vectorSearchMode = VectorSearchMode.EXACT_SCAN;
+ _runtimeFallbackReason = _requiredCandidateFallbackReason != null ?
_requiredCandidateFallbackReason
+ : "vector_index_not_filter_aware_for_mandatory_scope";
+
VectorSearchMetrics.getInstance().recordFallback(_runtimeFallbackReason);
+ LOGGER.warn("Performing exact allowed-document vector scan on column:
{} because {}. allowedDocs={}",
+ column, _runtimeFallbackReason,
effectiveAllowedDocIds.getCardinality());
+ Float threshold = _hasThresholdPredicate ? _distanceThreshold : null;
+ searchExecuted = true;
+ ImmutableRoaringBitmap exactResults =
ExactVectorScanFilterOperator.computeExactMatches(
+ _forwardIndexReader, queryVector, _predicate.getTopK(), _numDocs,
_distanceFunction, threshold,
+ effectiveAllowedDocIds, column);
+ _annCandidateCount = -1;
+ _rerankedCandidateCount = exactResults.getCardinality();
+ return exactResults;
+ }
+
// 1. Configure backend-specific parameters via interfaces
configureBackendParams(column);
- refreshExplainContext(null);
+ backendParamsConfigured = true;
+ refreshExplainContext();
explainContext = _vectorExplainContext;
Review Comment:
clearBackendParams(column) is now conditional on backendParamsConfigured,
but backendParamsConfigured is set only after configureBackendParams(column)
returns. If configureBackendParams throws after partially mutating reader state
(e.g., thread-local overrides), the cleanup won’t run. To make the code
exception-safe, consider setting the flag before invoking
configureBackendParams, or wrapping configure/clear in their own try/finally so
any partial configuration is reliably cleared.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/BaseFilterOperator.java:
##########
@@ -126,9 +127,30 @@ public FilteredDocIds getFilteredDocIds() {
return _filteredDocIds;
}
+ /// Returns the true-document set for execution, reusing an exact bitmap
that was already materialized through
+ /// [#getFilteredDocIds()] when available. Package-private so boolean filter
operators can avoid evaluating a scan
+ /// child twice without adding a new external API surface.
+ BlockDocIdSet getTruesForExecution() {
+ FilteredDocIds filteredDocIds = _filteredDocIds;
+ if (filteredDocIds == null) {
+ return getTrues();
+ }
+ ImmutableRoaringBitmap docIds = filteredDocIds.getDocIds();
+ if (docIds == null) {
+ return new MatchAllDocIdSet(_numDocs);
+ }
+ long numEntriesScannedInFilter =
filteredDocIds.getNumEntriesScannedInFilter();
+ return new BitmapDocIdSet(docIds, _numDocs) {
+ @Override
+ public long getNumEntriesScannedInFilter() {
+ return numEntriesScannedInFilter;
+ }
+ };
+ }
Review Comment:
When filteredDocIds.getDocIds() is null (match-all), getTruesForExecution()
returns MatchAllDocIdSet without preserving the already-accounted
getNumEntriesScannedInFilter() from the materialization. This can regress
filter accounting/metrics for scan-based operators that materialize to
match-all. Consider returning a MatchAllDocIdSet that overrides
getNumEntriesScannedInFilter() to return the captured numEntriesScannedInFilter
(similar to the BitmapDocIdSet wrapper).
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/VectorSimilarityFilterOperator.java:
##########
@@ -381,11 +523,14 @@ private ImmutableRoaringBitmap executeSearch() {
return annResults;
} finally {
- // Record search metrics for observability — always, regardless of which
path was taken
- VectorSearchMetrics.getInstance().recordSearch(_vectorSearchMode,
_backendType);
+ if (searchExecuted) {
+ VectorSearchMetrics.getInstance().recordSearch(_vectorSearchMode,
_backendType);
+ }
// Refresh explain context with the final search mode decided during
execution
- refreshExplainContext(null);
- clearBackendParams(column);
+ refreshExplainContext();
+ if (backendParamsConfigured) {
+ clearBackendParams(column);
+ }
Review Comment:
clearBackendParams(column) is now conditional on backendParamsConfigured,
but backendParamsConfigured is set only after configureBackendParams(column)
returns. If configureBackendParams throws after partially mutating reader state
(e.g., thread-local overrides), the cleanup won’t run. To make the code
exception-safe, consider setting the flag before invoking
configureBackendParams, or wrapping configure/clear in their own try/finally so
any partial configuration is reliably cleared.
--
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]
