raghavyadav01 commented on code in PR #19760:
URL: https://github.com/apache/pinot/pull/19760#discussion_r4199282762
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/MultiColumnTextIndexReader.java:
##########
@@ -41,6 +41,25 @@ default MutableRoaringBitmap getDocIds(String column, String
searchQuery) {
/// @return Matching document ids
MutableRoaringBitmap getDocIds(String column, String searchQuery, @Nullable
String optionsString);
+ /// Column-aware counterpart of [TextIndexReader#getNumMatchingDocs]:
returns the number of documents of
+ /// `column` matching the query without materializing their ids.
+ ///
+ /// Defaults to materializing, so an implementation that cannot count more
cheaply keeps its behaviour.
+ ///
+ /// @param column The column to query
+ /// @param searchQuery The search query string
+ /// @param optionsString Options string in format "key1=value1,key2=value2",
can be null
+ /// @return the number of matching documents
+ default int getNumMatchingDocs(String column, String searchQuery, @Nullable
String optionsString) {
Review Comment:
The multi-column call site in `TextMatchFilterOperator` moved from
`getDocIds(column, ...).getCardinality()` to this new default, and nothing
exercises it — `TextSearchMultiColIndexQueriesTest` has no `COUNT(*)` query at
all, and `FastFilteredCountTest` only uses a single-column text index. The
behaviour should be identical, but the override below is the one thing that
turns a working `COUNT(*)` into a runtime `UnsupportedOperationException` if
any caller ever reaches it, and today nothing proves it is unreachable. Could
you add a `COUNT(*) WHERE TEXT_MATCH(...)` case to
`TextSearchMultiColIndexQueriesTest` so this path is pinned before the
follow-up lands?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/LuceneTextIndexUtils.java:
##########
@@ -361,4 +371,55 @@ public String getMinimumShouldMatch() {
public static LuceneTextIndexOptions createOptions(String optionsString) {
return new LuceneTextIndexOptions(optionsString);
}
+
+ /// Counts the documents matching `query` without materializing their ids.
+ ///
+ /// A `count(*)` over a text filter only needs the size of the match set.
Collecting every matching doc id
+ /// into a bitmap first costs time proportional to the number of matches,
which on a frequent term over a
+ /// large segment is most of the query.
+ ///
+ /// Follows the recipe in Lucene's
[org.apache.lucene.search.IndexSearcher#count(Query)]: one rewrite and one
+ /// [Weight], then per leaf take [Weight#count] when the leaf can answer
from index metadata and iterate that
+ /// leaf only when it cannot. Probing per leaf rather than bailing out
wholesale means a query that is
+ /// countable on some leaves still pays iteration on the rest only.
+ ///
+ /// Wrapping in [ConstantScoreQuery] before the rewrite is NOT redundant
with [ScoreMode#COMPLETE_NO_SCORES]:
+ /// it is what makes several rewritten shapes produce count-friendly
weights, and mirrors `IndexSearcher#count`.
+ /// Removing it silently degrades those shapes to full iteration.
+ ///
+ /// The iterating branch keeps
[QueryThreadContext#checkTerminationAndSampleUsagePeriodically] so a slow count
+ /// still honours the query timeout. The metadata probe itself is not
interruptible, matching the rewrite that
+ /// already happens inside `IndexSearcher#search`.
+ ///
+ /// Counts are invariant under Pinot's Lucene-to-Pinot doc id translation,
which is 1:1, so no translator is
+ /// needed here. Callers that cannot guarantee that mapping must not use
this method.
+ ///
+ /// @return the number of matching documents
+ public static int countWithoutMaterializing(IndexSearcher searcher, Query
query)
+ throws IOException {
+ Query rewritten = searcher.rewrite(new ConstantScoreQuery(query));
+ Weight weight = searcher.createWeight(rewritten,
ScoreMode.COMPLETE_NO_SCORES, 1f);
+ int total = 0;
+ for (LeafReaderContext leaf : searcher.getIndexReader().leaves()) {
+ int leafCount = weight.count(leaf);
+ if (leafCount != -1) {
+ total += leafCount;
+ continue;
+ }
+ Scorer scorer = weight.scorer(leaf);
+ if (scorer == null) {
+ continue;
+ }
+ DocIdSetIterator iterator = scorer.iterator();
+ Bits liveDocs = leaf.reader().getLiveDocs();
+ int numCollected = 0;
+ for (int doc = iterator.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS;
doc = iterator.nextDoc()) {
+
QueryThreadContext.checkTerminationAndSampleUsagePeriodically(numCollected++,
"LuceneCount");
Review Comment:
This termination check is the one piece of new behaviour with no test — the
16 query shapes all complete, so nothing ever enters the loop and gets
interrupted. A test that sets a deadline already in the past (or a tiny
`timeoutMs`) around a query that falls into the iterating branch would pin the
claim in the javadoc above, and it's what would have surfaced the error-code
loss I flagged in `getNumMatchingDocs`. Could you add one?
--
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]
