You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "arunkumarucet (via GitHub)" <gi...@apache.org> on 2026/10/06 07:24:37 UTC
[PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
arunkumarucet opened a new pull request, #19760:
URL: https://github.com/apache/pinot/pull/19760
## Problem
A `count(*)` with a `TEXT_MATCH` filter only needs the size of the match set, but `TextMatchFilterOperator#getNumMatchingDocs` builds a full bitmap of every matching document and then reads its cardinality:
```java
return _textIndexReader.getDocIds(_predicate.getValue(), _predicate.getOptions()).getCardinality();
```
The cost of that grows with the number of matches, so it dominates a count over a high-frequency term on a large segment. Nothing survives the bitmap except a single integer.
The fast-count path already exists and is already wired up: `AggregationPlanNode` routes to `FastFilteredCountOperator` when a filter reports `canOptimizeCount()`, and `TextMatchFilterOperator` already returns `true`. Only the underlying implementation still materialized the bitmap.
## Change
- `TextIndexReader#getNumMatchingDocs(searchQuery, optionsString)`, defaulting to the existing `getDocIds(...).getCardinality()` so every other implementation is unaffected.
- `LuceneTextIndexReader` implements it with `Weight#count` per leaf. Lucene answers a single-term query from index metadata without visiting documents. Where a leaf cannot be counted cheaply it returns `-1`, and we fall back to the existing path rather than letting Lucene iterate internally, so the collector's query-termination check is preserved and a slow count still honours the query timeout.
- `TextMatchFilterOperator` asks for the count instead of the match set.
- Query parsing moves into one shared method, so the count path and the doc-id path cannot interpret the same query differently. Otherwise `count(*)` could disagree with the rows its own filter returns.
## Measurements
On a 1B-row OpenTelemetry log corpus (single node, 232 segments, Lucene text index on the log body), counting a high-frequency term took 183 ms, against 5 ms for an engine that reads the count from posting-list metadata. The gap tracked match count across queries:
| Query | Filter | Matches | Before |
|---|---|---|---|
| `count(*) WHERE TEXT_MATCH(Body, 'error')` | single term, high frequency | ~100M | 36.6x slower |
| `count(*) WHERE TEXT_MATCH(Body, 'failed AND order')` | conjunction | mid | 13.4x |
| `count(*) WHERE TEXT_MATCH(Body, 'payment')` | single term, low frequency | ~1.7M | 8.8x |
| 8-term conjunction | conjunction, low frequency | low | 3.8x |
The effect is not visible at small scale: on the same workload at 1M rows, materializing the bitmap is cheap enough that the difference disappears. That is likely why it has gone unnoticed.
## Tests
`LuceneTextIndexCountTest` pins the invariant that the short-circuit must return exactly what counting the materialized doc ids returns, across 16 query shapes: single terms (including one matching no documents and one matching every document), `AND`/`OR`/`NOT`, phrases, prefix, wildcard, fuzzy, regexp, and option-driven parsing (`parser=MATCH,minimumShouldMatch=2`, `allowLeadingWildcard=true`). This covers both the metadata path and the fallback.
All 61 existing text-index tests in `pinot-segment-local` pass.
## Follow-up
Multi-column text indexes keep the existing behaviour; `MultiColumnTextIndexReader` takes a different signature and is marked with a TODO.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19760:
URL: https://github.com/apache/pinot/pull/19760#issuecomment-6012449880
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19760?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`bbbed25`](https://app.codecov.io/gh/apache/pinot/commit/bbbed251444e00d58bdcbe59a6f233902eb7e79c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c59d716`](https://app.codecov.io/gh/apache/pinot/commit/c59d71613bcdf9cd9a8aaec9ad1277da0dd09e91?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 250 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19760 +/- ##
==============================================
+ Coverage 57.71% 100.00% +42.28%
+ Complexity 7 6 -1
==============================================
Files 2686 3 -2683
Lines 163987 6 -163981
Branches 26627 0 -26627
==============================================
- Hits 94640 6 -94634
+ Misses 61352 0 -61352
+ Partials 7995 0 -7995
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.28%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.28%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19760/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19760?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on PR #19760:
URL: https://github.com/apache/pinot/pull/19760#issuecomment-6014075573
Thanks — this was a genuinely useful review. All points addressed in `9cf23f5`, with one deliberately deferred.
**Single-pass counting (the slower-than-before regression).** You were right and this was the important one: every non-countable shape paid rewrite + `createWeight` twice. Rewritten to follow the `IndexSearcher#count` recipe — one rewrite, one `Weight`, per leaf either `Weight#count` or iteration of that leaf alone. That single change also resolved three of your other comments:
- the fallback no longer materializes a bitmap or does per-hit doc-id translation;
- `_countSearcher` is gone entirely, so there is no second searcher to keep in sync and no forfeited cached counts for `enableQueryCache=true` users (and you're right that my justifying comment misstated `onUse` — it fires per use, not per `createWeight`);
- the iterating branch keeps `checkTerminationAndSampleUsagePeriodically`.
**Corruption guard.** Added: `getNumMatchingDocs` falls back to `getDocIds` when `_indexReader.numDocs() > numDocs`, so a broken doc-id mapping keeps throwing from the translator instead of returning an inflated count. Thank you for tracing the reachable path.
**`buildQuery` javadoc.** You were right that it asserted an invariant nothing enforced. `getDocIds(String, String)` now routes through it too, which collapsed `getDocIdsWithOptions`/`getDocIdsWithoutOptions` into one method and makes the comment true rather than aspirational.
**Single-leaf fixture.** Correct, and it meant the only non-trivial logic was untested. Added a fixture built via `IndexWriter` with commits and no force-merge so `leaves() > 1`, and the whole query matrix now runs against it. Also added an assertion that a single term is countable from metadata — the precondition the optimization rests on, which nothing else would catch losing.
**`ConstantScoreQuery` wrap.** Documented as load-bearing with a pointer to `IndexSearcher#count`, per your note that a future "cleanup" would silently degrade countability without failing any test.
**Smaller items.** `INDEX_DIR` is now unique per run (agreed on the parallel-executor race), `Map.of()` replaces `new HashMap<>()`, and the class javadoc carries a thread-safety statement.
**Other three readers — deferred, deliberately.** I extracted the counting logic to `LuceneTextIndexUtils.countWithoutMaterializing(IndexSearcher, Query)` so adopting it elsewhere is small, and added the column-aware `MultiColumnTextIndexReader#getNumMatchingDocs` default so completing it never touches the filter operator again. I stopped short of converting `RealtimeLuceneTextIndex`, `MultiColumnLuceneTextIndexReader` and `MultiColumnRealtimeLuceneTextIndex` in this PR: the multi-column reader resolves parser configuration per column and the realtime ones need `SearcherManager` acquire/release, so each wants its own change and test. Happy to do them here instead if you'd prefer one PR. I did fix the landmine you spotted — the inherited single-column `getNumMatchingDocs` on a multi-column reader now throws the same clear message as `getDocIds` rather than the confusing one.
80/80 tests pass in `pinot-segment-local` (36 in the count suite, including the multi-leaf matrix), plus checkstyle, license and spotless.
On motivation, for context: this was found measuring a 1B-row OpenTelemetry log corpus, where counting a high-frequency term took 183 ms against 5 ms for an engine reading the count off posting-list metadata, and the gap scaled with match count (3.8x at low frequency, 36.6x on a term matching ~100M docs). It is invisible below ~10M rows, which is likely why it has survived.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on PR #19760:
URL: https://github.com/apache/pinot/pull/19760#issuecomment-6015511856
CI failure fixed in `9a1435f4d1`.
Merging `getDocIdsWithOptions` / `getDocIdsWithoutOptions` into one method (review comment on the `buildQuery` javadoc) also merged their exception messages, and the one that survived was the non-options wording. `TextSearchQueriesTest#testTextFilterOptimizerWithWildcardsDifferentOptions` asserts the message contains "Failed while searching the text index", so a leading-wildcard failure under `parser=STANDARD` started reporting unfamiliar text. Restored that wording for the merged method, with a comment naming the test so it does not get tidied away again. Nothing asserts the other phrasing.
My miss: the change is in `pinot-segment-local` so that is what I ran, but the callers and this assertion live in `pinot-core`. Now green locally across both: `TextSearchQueriesTest` 17, `TextSearchQueriesWithCombinedFilesTest` 17, `TextSearchMultiColIndexQueriesTest` 16, `TextMatchTransformFunctionTest` 9, `LuceneTextIndexCountTest` 36, plus the rest of the text suite in both modules.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on PR #19760:
URL: https://github.com/apache/pinot/pull/19760#issuecomment-6029863172
Thanks @raghavyadav01 — all seven addressed in `73d34a2`, with one correction to a premise at the end.
**Scorer loop giving up `BulkScorer`.** You're right, and this is the second time in this PR I optimised the metadata path and pessimised everything else — `Weight#count` answers only simple terms, so every conjunction, disjunction and phrase took the hand-rolled loop. Now an ordinary `search(query, collector)` with a counting `Collector` modelled on `TotalHitCountCollector`: the per-leaf `Weight#count` shortcut lives in `getLeafCollector` via `Collector#setWeight`, everything else keeps bulk scoring. Dropping the `ConstantScoreQuery` wrapper also fixes the duplicate `LRUQueryCache` entry you flagged — the count and doc-id paths now share one key. Termination converts to `CollectionTerminatedException` exactly as `LuceneDocIdCollector` does.
**`QueryException` wrapping.** Fixed — rethrown unwrapped before the generic wrap, so a timeout or accountant kill keeps its error code through `BaseSingleBlockCombineOperator`.
**`numDocs()` vs `maxDoc()`.** Fixed. You're right that the translator is indexed in `maxDoc()` space, so with deletions a broken index could slip past the guard into the count path.
**Error text for the no-options path.** Not intentional, and a fair catch — I'd traded that for one test's expectation without noticing it's the common path. Both original messages are restored, selected by a single `hasOptions` predicate that the query builder also uses, so the query built and the error reported can't disagree about which path ran.
**Identity translator in the fixture.** Correct — `TryOptimize` collapsed it to a no-op, so the invariant the whole change rests on was never tested. Added a reversed-order fixture giving a genuine permutation, asserting the count is unchanged by it, plus an understated `numDocs` case covering the `maxDoc` fallback.
**Untested termination check.** Added — but the test asserts something different from what I first wrote, because writing it corrected my understanding. On a plain deadline expiry `checkTerminationInternal` throws `EXECUTION_TIMEOUT` without recording a termination exception; only an accountant kill sets it. So asserting on `getTerminateException()` would have pinned a mechanism that doesn't apply to timeouts, in either path. The test now runs a phrase under an expired deadline and asserts counting is truncated *and* returns exactly what `getDocIds` returns under the same deadline — parity with the doc-id path, which is the contract that actually exists.
**Multi-column `COUNT(*)` coverage — one correction.** It is already exercised: `TextSearchMultiColIndexQueriesTest` extends `TextSearchQueriesTest` and inherits 16 of its 17 `@Test` methods (only `testMultiThreadedLuceneRealtime` is disabled), including `testInterSegment`, whose `count(*) ... TEXT_MATCH` assertions route through `FastFilteredCountOperator` into the column-aware default. But your underlying point stands — that coverage is invisible to anyone reading the multi-col file. Added a named `testCountStarWithTextMatchIsServedByTheFilterCount` to the shared parent, covering both a metadata-countable shape and one that must collect, so it runs visibly in both subclasses.
Green locally across both modules: `pinot-segment-local` 83, `pinot-core` 63 (the multi-col and parent suites both went up by one, confirming the new test runs in both), plus checkstyle, license and spotless.
Left as follow-up, unchanged: `RealtimeLuceneTextIndex`, `MultiColumnLuceneTextIndexReader` and `MultiColumnRealtimeLuceneTextIndex` still materialize. The static `LuceneTextIndexUtils#countWithoutMaterializing` is there so each is a small change, but the multi-column reader resolves parser config per column and the realtime ones need `SearcherManager` acquire/release, so each wants its own change and tests.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19760:
URL: https://github.com/apache/pinot/pull/19760#discussion_r4193808134
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -255,6 +278,64 @@ private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
}
}
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
+ try {
+ Query query = buildQuery(searchQuery, optionsString);
+ int count = countWithoutMaterializing(query);
+ if (count >= 0) {
+ return count;
+ }
+ // Lucene cannot count this query from metadata. Collect instead, reusing the query already built and the
+ // collector that carries the query-termination check, so a slow count still honours the query timeout.
+ MutableRoaringBitmap docIds = new MutableRoaringBitmap();
+ _indexSearcher.search(query, new LuceneDocIdCollector(docIds, _docIdTranslator));
+ return docIds.getCardinality();
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while counting matches in the text index for column:" + _column + " search query:"
+ + searchQuery, e);
+ }
+ }
+
+ /// Counts matches from index metadata alone, without visiting any document.
+ ///
+ /// Lucene can do this for some queries -- a single term in particular -- and reports that it cannot by
+ /// returning -1 for a leaf, in which case this returns -1 and the caller collects instead.
+ ///
+ /// @return the match count, or -1 if any leaf cannot be counted without materializing matches
+ @VisibleForTesting
+ int countWithoutMaterializing(Query query)
Review Comment:
`countWithoutMaterializing` bypasses `_docIdTranslator`, so a Lucene-docs/Pinot-docs mismatch now yields a silently wrong `COUNT(*)` where the old path failed loudly.
This is reproducible: on an index whose Lucene doc count exceeds the Pinot segment's `numDocs` (reachable today via on-disk segments converted by pre-#18885 Pinot after a crashed conversion retry), `getDocIds` throws `IndexOutOfBoundsException` from `DefaultDocIdTranslator`, but this fast path returns the inflated raw Lucene count (e.g. 8 on an index where `numDocs=4`). That turns detectable corruption into a wrong answer.
Cheap hardening: skip the fast path (or fail) when `_indexReader.numDocs() > numDocs`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -255,6 +278,64 @@ private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
}
}
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
+ try {
+ Query query = buildQuery(searchQuery, optionsString);
+ int count = countWithoutMaterializing(query);
Review Comment:
Every non-countable query shape (phrases, booleans, wildcard/prefix/fuzzy/regexp — common TEXT_MATCH shapes) now pays query rewrite + `createWeight` twice per `count(*)`: the probe on `_countSearcher` is thrown away on `-1`, and the fallback `_indexSearcher.search` rewrites from scratch. For e.g. `count(*) WHERE TEXT_MATCH(col, 'conn*')`, the multi-term rewrite enumerates the terms dictionary once in `countWithoutMaterializing` and again inside the fallback search — making these counts measurably **slower** than before this PR.
Lucene's own `IndexSearcher#count` avoids this with a `TotalHitCountCollector`-style design: one search, `weight.count(ctx)` probed per leaf inside `getLeafCollector` (throwing `CollectionTerminatedException` on countable leaves, counting collection otherwise). That gives one rewrite, one `Weight`, per-leaf hybrid counting — and no second searcher needed. Adopting it here would also resolve the `_countSearcher` duplication and the fallback's bitmap materialization (see other comments).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -67,6 +72,11 @@ public class LuceneTextIndexReader implements TextIndexReader {
private final IndexReader _indexReader;
private final Directory _indexDirectory;
private final IndexSearcher _indexSearcher;
+ /// Used only by [#getNumMatchingDocs]. Deliberately never query-cached: the caching policy counts
+ /// every Weight created, so probing on the shared searcher and then falling back to a search would
+ /// register two uses for one query and promote entries into [org.apache.lucene.search.LRUQueryCache]
+ /// at twice the intended rate -- the heap behaviour the query cache is disabled by default to avoid.
+ private final IndexSearcher _countSearcher;
Review Comment:
A few issues with the dedicated `_countSearcher`:
- In the default config (query cache already disabled) it's a byte-for-byte duplicate of `_indexSearcher`.
- For users who set `enableQueryCache=true`, it unconditionally forfeits cached counts (Lucene 9.12's `CachingWrapperWeight.count` serves cached `CacheAndCount` entries), so repeated counts get strictly slower than before the PR.
- The justifying comment misstates the mechanism: `UsageTrackingQueryCachingPolicy.onUse` fires per *use* of a weight, not per `createWeight`.
- Two searcher fields must now be kept in sync across both constructors for any future searcher-level setting, or the count path silently diverges from the filter path.
Simpler: `_countSearcher = config.isEnableQueryCache() ? uncachedSearcher : _indexSearcher` — or eliminate the field entirely via the single-pass counting-collector design (see comment on `getNumMatchingDocs`), which creates exactly one `Weight`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -255,6 +278,64 @@ private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
}
}
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
+ try {
+ Query query = buildQuery(searchQuery, optionsString);
+ int count = countWithoutMaterializing(query);
+ if (count >= 0) {
+ return count;
+ }
+ // Lucene cannot count this query from metadata. Collect instead, reusing the query already built and the
+ // collector that carries the query-termination check, so a slow count still honours the query timeout.
+ MutableRoaringBitmap docIds = new MutableRoaringBitmap();
+ _indexSearcher.search(query, new LuceneDocIdCollector(docIds, _docIdTranslator));
+ return docIds.getCardinality();
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while counting matches in the text index for column:" + _column + " search query:"
+ + searchQuery, e);
+ }
+ }
+
+ /// Counts matches from index metadata alone, without visiting any document.
+ ///
+ /// Lucene can do this for some queries -- a single term in particular -- and reports that it cannot by
+ /// returning -1 for a leaf, in which case this returns -1 and the caller collects instead.
+ ///
+ /// @return the match count, or -1 if any leaf cannot be counted without materializing matches
+ @VisibleForTesting
+ int countWithoutMaterializing(Query query)
+ throws IOException {
+ Weight weight = _countSearcher.createWeight(_countSearcher.rewrite(new ConstantScoreQuery(query)),
+ ScoreMode.COMPLETE_NO_SCORES, 1f);
+ int total = 0;
+ for (LeafReaderContext leaf : _indexReader.leaves()) {
+ int leafCount = weight.count(leaf);
+ if (leafCount == -1) {
+ return -1;
+ }
+ total += leafCount;
+ }
+ return total;
+ }
+
+ /// Parses a search string into a Lucene query, honouring the options string when one is given.
+ ///
+ /// Shared by the doc-id and the count paths so that a `count(*)` and the filter it counts can never
Review Comment:
This javadoc claims the method is "Shared by the doc-id and the count paths", but the doc-id path never calls it — `getDocIds(String, String)` performs the same options dispatch independently. So the invariant the comment promises (count and filter can never interpret a query differently) is not actually enforced: a maintainer could edit options handling here, trust the comment, and skip `getDocIds` — shipping exactly the count-vs-filter divergence this comment says is impossible. `LuceneTextIndexCountTest` can't catch that, because the fallback uses `buildQuery` too, making the comparison a tautology.
Suggest routing `getDocIds(String, String)` through `buildQuery(searchQuery, optionsString)` (which would also collapse `getDocIdsWithOptions`/`getDocIdsWithoutOptions`), or correcting the comments on both overloads (the 1-arg overload's comment also says it applies "options", which it doesn't).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -255,6 +278,64 @@ private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
}
}
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
+ try {
+ Query query = buildQuery(searchQuery, optionsString);
+ int count = countWithoutMaterializing(query);
+ if (count >= 0) {
+ return count;
+ }
+ // Lucene cannot count this query from metadata. Collect instead, reusing the query already built and the
Review Comment:
The fallback still materializes the full doc-id bitmap plus a per-hit `DocIdTranslator` lookup just to read `getCardinality()` — the exact per-match cost this PR exists to remove — and is a third in-class copy of the bitmap + `LuceneDocIdCollector` + search idiom. A count-only `LeafCollector` keeping the same `QueryThreadContext.checkTerminationAndSampleUsagePeriodically` check but incrementing an `int` preserves the timeout behaviour and skips both; translation is cardinality-invariant (1:1 mapping).
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/TextMatchFilterOperator.java:
##########
@@ -79,10 +79,13 @@ public boolean canOptimizeCount() {
@Override
public int getNumMatchingDocs() {
if (_textIndexReader.isMultiColumn()) {
+ // TODO: give MultiColumnTextIndexReader the same count short-circuit.
return ((MultiColumnTextIndexReader) _textIndexReader).getDocIds(_column, _predicate.getValue(),
_predicate.getOptions()).getCardinality();
} else {
Review Comment:
Rather than an operator-side TODO, a two-line default method on `MultiColumnTextIndexReader` — `getNumMatchingDocs(column, query, options)` defaulting to `getDocIds(...).getCardinality()` — would put both branches at the same altitude, so completing the TODO only touches the reader, not this operator again.
Also a latent landmine worth noting: `MultiColumnLuceneTextIndexReader` inherits the new single-column `getNumMatchingDocs` default, whose delegate `getDocIds` throws `UnsupportedOperationException("Multi-column text index requires column name to query!")` if any future caller invokes it generically.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexCountTest.java:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.text;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.segment.local.segment.creator.impl.text.LuceneTextIndexCreator;
+import org.apache.pinot.segment.local.segment.index.text.TextIndexConfigBuilder;
+import org.apache.pinot.segment.spi.index.TextIndexConfig;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+/// Pins the invariant behind the `count(*)` short-circuit: [LuceneTextIndexReader#getNumMatchingDocs] must
+/// return exactly what counting the materialized doc ids would return, for every query shape.
+///
+/// The short-circuit reads a count from Lucene's index metadata where it can and falls back to materializing
+/// doc ids where it cannot, so the two paths have to agree or a `count(*)` would silently disagree with the
+/// rows its own filter returns.
+public class LuceneTextIndexCountTest {
+ private static final File INDEX_DIR =
Review Comment:
`INDEX_DIR` is a fixed, non-unique path under the shared system temp directory. Two concurrent executions on the same host (parallel CI executors or surefire forks sharing `/tmp`) race: one JVM's `@BeforeClass` `deleteDirectory` removes the other's live mmapped index mid-test, causing intermittent `NoSuchFileException`/wrong-count flakes. Suggest appending a UUID/nanoTime suffix as other Pinot index tests do.
Minor, while here: the class javadoc describes behavior but omits a thread-safety statement (repo convention for new classes), which is relevant given the static shared `INDEX_DIR`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -255,6 +278,64 @@ private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
}
}
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
+ try {
+ Query query = buildQuery(searchQuery, optionsString);
+ int count = countWithoutMaterializing(query);
+ if (count >= 0) {
+ return count;
+ }
+ // Lucene cannot count this query from metadata. Collect instead, reusing the query already built and the
+ // collector that carries the query-termination check, so a slow count still honours the query timeout.
+ MutableRoaringBitmap docIds = new MutableRoaringBitmap();
+ _indexSearcher.search(query, new LuceneDocIdCollector(docIds, _docIdTranslator));
+ return docIds.getCardinality();
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while counting matches in the text index for column:" + _column + " search query:"
+ + searchQuery, e);
+ }
+ }
+
+ /// Counts matches from index metadata alone, without visiting any document.
+ ///
+ /// Lucene can do this for some queries -- a single term in particular -- and reports that it cannot by
+ /// returning -1 for a leaf, in which case this returns -1 and the caller collects instead.
+ ///
+ /// @return the match count, or -1 if any leaf cannot be counted without materializing matches
+ @VisibleForTesting
+ int countWithoutMaterializing(Query query)
+ throws IOException {
+ Weight weight = _countSearcher.createWeight(_countSearcher.rewrite(new ConstantScoreQuery(query)),
Review Comment:
Two notes on this line:
1. The `ConstantScoreQuery` wrap before rewrite looks redundant next to `ScoreMode.COMPLETE_NO_SCORES`, but it's load-bearing — it mirrors Lucene's `IndexSearcher#count` recipe for producing count-friendly weights. Nothing in the code says so, and a future cleanup that removes the "redundant" wrap wouldn't fail any test: some rewritten shapes would just stop producing countable weights and silently degrade to full collection (which `testFallbackPathIsExercised` is explicitly designed not to catch). Worth one comment line pointing at `IndexSearcher#count`.
2. Minor: the metadata-count probe (rewrite + `createWeight` + per-leaf `Weight.count`) runs outside Pinot's query-termination and resource-usage accounting, so the in-code claim that a slow count "still honours the query timeout" is only true of the fallback half. A regexp or leading-wildcard count over a huge terms dictionary does bounded but non-trivial per-leaf term enumeration during rewrite that the query killer can't interrupt. Low severity — the old path's rewrite inside `search()` was equally unchecked — but the probe doubles that unaccounted work for non-countable shapes.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexCountTest.java:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.text;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.segment.local.segment.creator.impl.text.LuceneTextIndexCreator;
+import org.apache.pinot.segment.local.segment.index.text.TextIndexConfigBuilder;
+import org.apache.pinot.segment.spi.index.TextIndexConfig;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+/// Pins the invariant behind the `count(*)` short-circuit: [LuceneTextIndexReader#getNumMatchingDocs] must
+/// return exactly what counting the materialized doc ids would return, for every query shape.
+///
+/// The short-circuit reads a count from Lucene's index metadata where it can and falls back to materializing
+/// doc ids where it cannot, so the two paths have to agree or a `count(*)` would silently disagree with the
+/// rows its own filter returns.
+public class LuceneTextIndexCountTest {
+ private static final File INDEX_DIR =
+ new File(FileUtils.getTempDirectory(), LuceneTextIndexCountTest.class.getSimpleName());
+ private static final String COLUMN = "body";
+ /// Every document carries `sentinelall` so there is a term matching all documents. `to` cannot serve that
+ /// role: it is in Pinot's default English stop-word set and matches nothing.
+ private static final String[] DOCS = {
+ "failed to place order for user alice sentinelall",
+ "failed to charge card for user bob sentinelall",
+ "connection refused by payment service sentinelall",
+ "connection established to cache service sentinelall",
+ "order confirmation email sent to carol sentinelall",
+ "cache miss while loading order details sentinelall",
+ "payment accepted for order 4815 sentinelall",
+ "unrelated housekeeping log line sentinelall"
+ };
+
+ @BeforeClass
+ public void setUp()
+ throws IOException {
+ FileUtils.deleteDirectory(INDEX_DIR);
+ FileUtils.forceMkdir(INDEX_DIR);
+ TextIndexConfig config = new TextIndexConfigBuilder().build();
+ try (LuceneTextIndexCreator creator = new LuceneTextIndexCreator(COLUMN, INDEX_DIR, true, false, null, null,
+ config)) {
+ for (String doc : DOCS) {
+ creator.add(doc);
+ }
+ creator.seal();
Review Comment:
This fixture is always a single-leaf index: `LuceneTextIndexCreator.seal()` calls `forceMerge(1)`. That means `countWithoutMaterializing`'s only non-trivial logic — cross-leaf accumulation and the mid-loop `return -1` — is never exercised by any test. A regression in the leaf loop (`break`/`continue` instead of `return -1`, or dropped accumulation) would pass this whole suite and then return a partial sum instead of falling back on any multi-leaf index, producing a silently short `count(*)`.
Suggest adding a fixture built via `IndexWriter` with commits between batches (no force-merge) so `leaves() > 1`, asserting both the summed count and the `-1` bail.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -227,26 +241,35 @@ public MutableRoaringBitmap getDocIds(String searchQuery, @Nullable String optio
return getDocIdsWithoutOptions(searchQuery);
}
+ /// Parses a search string into a Lucene query, applying the configured parser and options.
+ ///
+ /// Shared by the doc-id and the count paths so that a `count(*)` and the filter it counts can never
+ /// interpret the same query differently.
+ private Query buildQuery(String searchQuery)
Review Comment:
The count short-circuit lives as a private method in one of four near-identical Lucene readers; `RealtimeLuceneTextIndex`, `MultiColumnLuceneTextIndexReader`, and `MultiColumnRealtimeLuceneTextIndex` keep materializing, so the optimization is largely invisible on realtime-heavy and multi-column tables (a hybrid table's `count(*)` stays dominated by consuming segments' full bitmap builds, plus per-doc stored-field lookups in `RealtimeLuceneTextIndex.getPinotDocIds`). A static `LuceneTextIndexUtils.countWithoutMaterializing(IndexSearcher, Query)` would let all four readers adopt it in a few lines, since a count never needs doc-id translation — the very duplication the TODO at the bottom of this file documents.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexCountTest.java:
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.text;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.segment.local.segment.creator.impl.text.LuceneTextIndexCreator;
+import org.apache.pinot.segment.local.segment.index.text.TextIndexConfigBuilder;
+import org.apache.pinot.segment.spi.index.TextIndexConfig;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+/// Pins the invariant behind the `count(*)` short-circuit: [LuceneTextIndexReader#getNumMatchingDocs] must
+/// return exactly what counting the materialized doc ids would return, for every query shape.
+///
+/// The short-circuit reads a count from Lucene's index metadata where it can and falls back to materializing
+/// doc ids where it cannot, so the two paths have to agree or a `count(*)` would silently disagree with the
+/// rows its own filter returns.
+public class LuceneTextIndexCountTest {
+ private static final File INDEX_DIR =
+ new File(FileUtils.getTempDirectory(), LuceneTextIndexCountTest.class.getSimpleName());
+ private static final String COLUMN = "body";
+ /// Every document carries `sentinelall` so there is a term matching all documents. `to` cannot serve that
+ /// role: it is in Pinot's default English stop-word set and matches nothing.
+ private static final String[] DOCS = {
+ "failed to place order for user alice sentinelall",
+ "failed to charge card for user bob sentinelall",
+ "connection refused by payment service sentinelall",
+ "connection established to cache service sentinelall",
+ "order confirmation email sent to carol sentinelall",
+ "cache miss while loading order details sentinelall",
+ "payment accepted for order 4815 sentinelall",
+ "unrelated housekeeping log line sentinelall"
+ };
+
+ @BeforeClass
+ public void setUp()
+ throws IOException {
+ FileUtils.deleteDirectory(INDEX_DIR);
+ FileUtils.forceMkdir(INDEX_DIR);
+ TextIndexConfig config = new TextIndexConfigBuilder().build();
+ try (LuceneTextIndexCreator creator = new LuceneTextIndexCreator(COLUMN, INDEX_DIR, true, false, null, null,
+ config)) {
+ for (String doc : DOCS) {
+ creator.add(doc);
+ }
+ creator.seal();
+ }
+ }
+
+ @AfterClass
+ public void tearDown()
+ throws IOException {
+ FileUtils.deleteDirectory(INDEX_DIR);
+ }
+
+ @DataProvider(name = "queries")
+ public Object[][] queries() {
+ return new Object[][]{
+ // Single terms: the shape Lucene can count from metadata alone, i.e. the case the optimization targets.
+ {"order", null},
+ {"connection", null},
+ {"housekeeping", null},
+ // No match, and a term in every document: the boundary cases.
+ {"nonexistentterm", null},
+ {"sentinelall", null},
+ // A stop word, which the default analyzer drops entirely.
+ {"to", null},
+ // Boolean, phrase and multi-term shapes, which generally take the fallback.
+ {"failed AND order", null},
+ {"failed OR connection", null},
+ {"order AND NOT failed", null},
+ {"\"place order\"", null},
+ {"\"failed to place order\"", null},
+ // Prefix, wildcard, fuzzy and regexp.
+ {"conn*", null},
+ {"c?che", null},
+ {"connection~1", null},
+ {"/conn.*/", null},
+ // Option-driven parsing must agree too.
+ {"failed order connection", "parser=MATCH,minimumShouldMatch=2"},
+ {"*tion", "allowLeadingWildcard=true"}
+ };
+ }
+
+ @Test(dataProvider = "queries")
+ public void testCountMatchesMaterializedDocIds(String query, String options)
+ throws IOException {
+ try (LuceneTextIndexReader reader = new LuceneTextIndexReader(COLUMN, INDEX_DIR, DOCS.length, new HashMap<>())) {
Review Comment:
Nit: `new HashMap<>()` (here and in the other three tests) can be `Map.of()` per the repo convention — the value flows only to `TextIndexConfigBuilder.withProperties()`, which never mutates it.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on PR #19760:
URL: https://github.com/apache/pinot/pull/19760#issuecomment-6075525329
@raghavyadav01 @xiangfu0 Can anyone of you pls approve and merge this? TIA
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #19760:
URL: https://github.com/apache/pinot/pull/19760#discussion_r4199239588
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -218,60 +224,80 @@ public MutableRoaringBitmap getDocIds(String searchQuery) {
@Override
public MutableRoaringBitmap getDocIds(String searchQuery, @Nullable String optionsString) {
- if (optionsString != null && !optionsString.trim().isEmpty()) {
- LuceneTextIndexUtils.LuceneTextIndexOptions options = LuceneTextIndexUtils.createOptions(optionsString);
- if (!options.getOptions().isEmpty()) {
- return getDocIdsWithOptions(searchQuery, options);
- }
- }
- return getDocIdsWithoutOptions(searchQuery);
- }
-
- private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
MutableRoaringBitmap docIds = new MutableRoaringBitmap();
Collector docIDCollector = new LuceneDocIdCollector(docIds, _docIdTranslator);
try {
- // Lucene query parsers are generally stateful and a new instance must be created per query.
- QueryParserBase parser = _queryParserClassConstructor.newInstance(_column, _analyzer);
- // Phrase search with prefix/suffix matching may have leading *. E.g., `*pache pinot` which can be stripped by
- // the query parser. To support the feature, we need to explicitly set the config to be true.
- if (_enablePrefixSuffixMatchingInPhraseQueries) {
- parser.setAllowLeadingWildcard(true);
- }
- if (_useANDForMultiTermQueries) {
- parser.setDefaultOperator(QueryParser.Operator.AND);
- }
- Query query = parser.parse(searchQuery);
- if (_queryParserClass.equals("org.apache.lucene.queryparser.classic.QueryParser")
- && _enablePrefixSuffixMatchingInPhraseQueries) {
- query = LuceneTextIndexUtils.convertToMultiTermSpanQuery(query);
- }
- _indexSearcher.search(query, docIDCollector);
+ _indexSearcher.search(buildQuery(searchQuery, optionsString), docIDCollector);
return docIds;
} catch (Exception e) {
- String msg =
- "Caught exception while searching the text index for column:" + _column + " search query:" + searchQuery;
- throw new RuntimeException(msg, e);
+ // Wording matches what the options path threw before the two paths were merged; it is asserted on in
+ // TextSearchQueriesTest#testTextFilterOptimizerWithWildcardsDifferentOptions.
+ throw new RuntimeException(
+ "Failed while searching the text index for column " + _column + " with search query: " + searchQuery, e);
}
}
- // TODO: Consider creating a base class (e.g., BaseLuceneTextIndexReader) to avoid code duplication
- // for getDocIdsWithOptions method across LuceneTextIndexReader, MultiColumnLuceneTextIndexReader,
- // RealtimeLuceneTextIndex, and MultiColumnRealtimeLuceneTextIndex
- private MutableRoaringBitmap getDocIdsWithOptions(String actualQuery,
- LuceneTextIndexUtils.LuceneTextIndexOptions options) {
- MutableRoaringBitmap docIds = new MutableRoaringBitmap();
- Collector docIDCollector = new LuceneDocIdCollector(docIds, _docIdTranslator);
+ /// Parses a search string into a Lucene query, applying the configured parser and options.
+ ///
+ /// Shared by the doc-id and the count paths so that a `count(*)` and the filter it counts can never
+ /// interpret the same query differently.
+ private Query buildQuery(String searchQuery)
+ throws Exception {
+ // Lucene query parsers are generally stateful and a new instance must be created per query.
+ QueryParserBase parser = _queryParserClassConstructor.newInstance(_column, _analyzer);
+ // Phrase search with prefix/suffix matching may have leading *. E.g., `*pache pinot` which can be stripped by
+ // the query parser. To support the feature, we need to explicitly set the config to be true.
+ if (_enablePrefixSuffixMatchingInPhraseQueries) {
+ parser.setAllowLeadingWildcard(true);
+ }
+ if (_useANDForMultiTermQueries) {
+ parser.setDefaultOperator(QueryParser.Operator.AND);
+ }
+ Query query = parser.parse(searchQuery);
+ if (_queryParserClass.equals("org.apache.lucene.queryparser.classic.QueryParser")
+ && _enablePrefixSuffixMatchingInPhraseQueries) {
+ query = LuceneTextIndexUtils.convertToMultiTermSpanQuery(query);
+ }
+ return query;
+ }
+
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
try {
- Query query = LuceneTextIndexUtils.createQueryParserWithOptions(actualQuery, options, _column, _analyzer);
- _indexSearcher.search(query, docIDCollector);
- return docIds;
+ // A Lucene index with more documents than the segment means the doc-id mapping is broken -- a state
+ // getDocIds surfaces as an IndexOutOfBoundsException from the translator. Counting does not consult the
+ // translator, so without this guard the corruption would become a silently inflated count instead.
+ // Fall back so that it keeps failing loudly.
+ if (_indexReader.numDocs() > _numDocs) {
+ return getDocIds(searchQuery, optionsString).getCardinality();
+ }
+ return LuceneTextIndexUtils.countWithoutMaterializing(_indexSearcher,
+ buildQuery(searchQuery, optionsString));
} catch (Exception e) {
throw new RuntimeException(
- "Failed while searching the text index for column " + _column + " with search query: " + actualQuery, e);
+ "Caught exception while counting matches in the text index for column:" + _column + " search query:"
Review Comment:
`checkTerminationAndSampleUsagePeriodically` throws `TerminationException`, which is a `QueryException` — wrapping it here turns a timeout or an accountant kill into a generic `QUERY_EXECUTION` error, since `BaseSingleBlockCombineOperator.onProcessSegmentsException` only preserves the error code for `QueryException`. That's exactly what the `CollectionTerminatedException` conversion in `LuceneDocIdCollector` exists to avoid, so the count path loses the termination reason the doc-id path keeps. Can you rethrow `QueryException` unwrapped before the generic wrap?
##########
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);
Review Comment:
Lucene 9.12's `IndexSearcher#count` doesn't iterate a `Scorer` by hand on the fallback — it does `search(new ConstantScoreQuery(q), new TotalHitCountCollectorManager(...))`, which keeps the `BulkScorer` path. `Weight#count` returns -1 for anything but simple terms, so every conjunction, disjunction and phrase lands in this branch and gives up bulk scoring that the old `search(query, LuceneDocIdCollector)` was getting; a wide OR could come out slower than the bitmap it replaced.
Would a counting `Collector` modelled on `LuceneDocIdCollector` work here instead — same `CollectionTerminatedException` handling, with the `weight.count(leaf)` shortcut in `getLeafCollector` the way `TotalHitCountCollector` does it? It would also keep the query-cache key identical to the doc-id path; right now the `ConstantScoreQuery` wrapper means the same predicate gets two entries in the shared `LRUQueryCache` when `enableQueryCache` is on.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -218,60 +224,80 @@ public MutableRoaringBitmap getDocIds(String searchQuery) {
@Override
public MutableRoaringBitmap getDocIds(String searchQuery, @Nullable String optionsString) {
- if (optionsString != null && !optionsString.trim().isEmpty()) {
- LuceneTextIndexUtils.LuceneTextIndexOptions options = LuceneTextIndexUtils.createOptions(optionsString);
- if (!options.getOptions().isEmpty()) {
- return getDocIdsWithOptions(searchQuery, options);
- }
- }
- return getDocIdsWithoutOptions(searchQuery);
- }
-
- private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
MutableRoaringBitmap docIds = new MutableRoaringBitmap();
Collector docIDCollector = new LuceneDocIdCollector(docIds, _docIdTranslator);
try {
- // Lucene query parsers are generally stateful and a new instance must be created per query.
- QueryParserBase parser = _queryParserClassConstructor.newInstance(_column, _analyzer);
- // Phrase search with prefix/suffix matching may have leading *. E.g., `*pache pinot` which can be stripped by
- // the query parser. To support the feature, we need to explicitly set the config to be true.
- if (_enablePrefixSuffixMatchingInPhraseQueries) {
- parser.setAllowLeadingWildcard(true);
- }
- if (_useANDForMultiTermQueries) {
- parser.setDefaultOperator(QueryParser.Operator.AND);
- }
- Query query = parser.parse(searchQuery);
- if (_queryParserClass.equals("org.apache.lucene.queryparser.classic.QueryParser")
- && _enablePrefixSuffixMatchingInPhraseQueries) {
- query = LuceneTextIndexUtils.convertToMultiTermSpanQuery(query);
- }
- _indexSearcher.search(query, docIDCollector);
+ _indexSearcher.search(buildQuery(searchQuery, optionsString), docIDCollector);
return docIds;
} catch (Exception e) {
- String msg =
- "Caught exception while searching the text index for column:" + _column + " search query:" + searchQuery;
- throw new RuntimeException(msg, e);
+ // Wording matches what the options path threw before the two paths were merged; it is asserted on in
+ // TextSearchQueriesTest#testTextFilterOptimizerWithWildcardsDifferentOptions.
+ throw new RuntimeException(
+ "Failed while searching the text index for column " + _column + " with search query: " + searchQuery, e);
}
}
- // TODO: Consider creating a base class (e.g., BaseLuceneTextIndexReader) to avoid code duplication
- // for getDocIdsWithOptions method across LuceneTextIndexReader, MultiColumnLuceneTextIndexReader,
- // RealtimeLuceneTextIndex, and MultiColumnRealtimeLuceneTextIndex
- private MutableRoaringBitmap getDocIdsWithOptions(String actualQuery,
- LuceneTextIndexUtils.LuceneTextIndexOptions options) {
- MutableRoaringBitmap docIds = new MutableRoaringBitmap();
- Collector docIDCollector = new LuceneDocIdCollector(docIds, _docIdTranslator);
+ /// Parses a search string into a Lucene query, applying the configured parser and options.
+ ///
+ /// Shared by the doc-id and the count paths so that a `count(*)` and the filter it counts can never
+ /// interpret the same query differently.
+ private Query buildQuery(String searchQuery)
+ throws Exception {
+ // Lucene query parsers are generally stateful and a new instance must be created per query.
+ QueryParserBase parser = _queryParserClassConstructor.newInstance(_column, _analyzer);
+ // Phrase search with prefix/suffix matching may have leading *. E.g., `*pache pinot` which can be stripped by
+ // the query parser. To support the feature, we need to explicitly set the config to be true.
+ if (_enablePrefixSuffixMatchingInPhraseQueries) {
+ parser.setAllowLeadingWildcard(true);
+ }
+ if (_useANDForMultiTermQueries) {
+ parser.setDefaultOperator(QueryParser.Operator.AND);
+ }
+ Query query = parser.parse(searchQuery);
+ if (_queryParserClass.equals("org.apache.lucene.queryparser.classic.QueryParser")
+ && _enablePrefixSuffixMatchingInPhraseQueries) {
+ query = LuceneTextIndexUtils.convertToMultiTermSpanQuery(query);
+ }
+ return query;
+ }
+
+ @Override
+ public int getNumMatchingDocs(String searchQuery, @Nullable String optionsString) {
try {
- Query query = LuceneTextIndexUtils.createQueryParserWithOptions(actualQuery, options, _column, _analyzer);
- _indexSearcher.search(query, docIDCollector);
- return docIds;
+ // A Lucene index with more documents than the segment means the doc-id mapping is broken -- a state
+ // getDocIds surfaces as an IndexOutOfBoundsException from the translator. Counting does not consult the
+ // translator, so without this guard the corruption would become a silently inflated count instead.
+ // Fall back so that it keeps failing loudly.
+ if (_indexReader.numDocs() > _numDocs) {
Review Comment:
The translator is indexed by `context.docBase + doc` in `LuceneDocIdCollector`, which is `maxDoc()` space, so `maxDoc()` is what actually trips the `IndexOutOfBounds` this guard stands in for. Once a leaf has deletions `numDocs() < maxDoc()`, and an index that would blow up in `getDocIds` can still slip past here into the count path. Can we compare against `maxDoc()` instead?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -218,60 +224,80 @@ public MutableRoaringBitmap getDocIds(String searchQuery) {
@Override
public MutableRoaringBitmap getDocIds(String searchQuery, @Nullable String optionsString) {
- if (optionsString != null && !optionsString.trim().isEmpty()) {
- LuceneTextIndexUtils.LuceneTextIndexOptions options = LuceneTextIndexUtils.createOptions(optionsString);
- if (!options.getOptions().isEmpty()) {
- return getDocIdsWithOptions(searchQuery, options);
- }
- }
- return getDocIdsWithoutOptions(searchQuery);
- }
-
- private MutableRoaringBitmap getDocIdsWithoutOptions(String searchQuery) {
MutableRoaringBitmap docIds = new MutableRoaringBitmap();
Collector docIDCollector = new LuceneDocIdCollector(docIds, _docIdTranslator);
try {
- // Lucene query parsers are generally stateful and a new instance must be created per query.
- QueryParserBase parser = _queryParserClassConstructor.newInstance(_column, _analyzer);
- // Phrase search with prefix/suffix matching may have leading *. E.g., `*pache pinot` which can be stripped by
- // the query parser. To support the feature, we need to explicitly set the config to be true.
- if (_enablePrefixSuffixMatchingInPhraseQueries) {
- parser.setAllowLeadingWildcard(true);
- }
- if (_useANDForMultiTermQueries) {
- parser.setDefaultOperator(QueryParser.Operator.AND);
- }
- Query query = parser.parse(searchQuery);
- if (_queryParserClass.equals("org.apache.lucene.queryparser.classic.QueryParser")
- && _enablePrefixSuffixMatchingInPhraseQueries) {
- query = LuceneTextIndexUtils.convertToMultiTermSpanQuery(query);
- }
- _indexSearcher.search(query, docIDCollector);
+ _indexSearcher.search(buildQuery(searchQuery, optionsString), docIDCollector);
return docIds;
} catch (Exception e) {
- String msg =
- "Caught exception while searching the text index for column:" + _column + " search query:" + searchQuery;
- throw new RuntimeException(msg, e);
+ // Wording matches what the options path threw before the two paths were merged; it is asserted on in
+ // TextSearchQueriesTest#testTextFilterOptimizerWithWildcardsDifferentOptions.
+ throw new RuntimeException(
+ "Failed while searching the text index for column " + _column + " with search query: " + searchQuery, e);
Review Comment:
Merging the two paths also changes the error text for the no-options path, which has been `Caught exception while searching the text index for column:...` up to now. Harmless for the test you cite, but this is the common path and it's the kind of string people end up alerting on — intentional?
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexCountTest.java:
##########
@@ -0,0 +1,199 @@
+/**
+ * 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.text;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.commons.io.FileUtils;
+import org.apache.lucene.analysis.standard.StandardAnalyzer;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.TextField;
+import org.apache.lucene.index.DirectoryReader;
+import org.apache.lucene.index.IndexWriter;
+import org.apache.lucene.index.IndexWriterConfig;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.Term;
+import org.apache.lucene.search.ConstantScoreQuery;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.ScoreMode;
+import org.apache.lucene.search.TermQuery;
+import org.apache.lucene.search.Weight;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.store.FSDirectory;
+import org.apache.pinot.segment.local.segment.creator.impl.text.LuceneTextIndexCreator;
+import org.apache.pinot.segment.local.segment.index.text.TextIndexConfigBuilder;
+import org.apache.pinot.segment.local.utils.LuceneTextIndexUtils;
+import org.apache.pinot.segment.spi.index.TextIndexConfig;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+/// Covers the `count(*)` short-circuit: [LuceneTextIndexReader#getNumMatchingDocs] and the
+/// [LuceneTextIndexUtils#countWithoutMaterializing] it delegates to.
+///
+/// Not thread-safe: the fixtures write to per-instance temp directories and are shared across the test
+/// methods of one instance, which TestNG runs single-threaded by default.
+public class LuceneTextIndexCountTest {
+ // Unique per run: a fixed path under the shared temp dir races other executors/surefire forks, whose
+ // @BeforeClass cleanup would delete a live mmapped index out from under this one.
+ private static final File TEMP_DIR =
+ new File(FileUtils.getTempDirectory(), LuceneTextIndexCountTest.class.getSimpleName() + "-" + UUID.randomUUID());
+ private static final File INDEX_DIR = new File(TEMP_DIR, "single-leaf");
+ private static final File MULTI_LEAF_DIR = new File(TEMP_DIR, "multi-leaf");
+ private static final String COLUMN = "body";
+
+ /// Every document carries `sentinelall` so there is a term matching all documents. `to` cannot serve that
+ /// role: it is in the default English stop-word set and matches nothing.
+ private static final String[] DOCS = {
+ "failed to place order for user alice sentinelall",
+ "failed to charge card for user bob sentinelall",
+ "connection refused by payment service sentinelall",
+ "connection established to cache service sentinelall",
+ "order confirmation email sent to carol sentinelall",
+ "cache miss while loading order details sentinelall",
+ "payment accepted for order 4815 sentinelall",
+ "unrelated housekeeping log line sentinelall"
+ };
+
+ @BeforeClass
+ public void setUp()
+ throws IOException {
+ FileUtils.forceMkdir(INDEX_DIR);
+ TextIndexConfig config = new TextIndexConfigBuilder().build();
+ try (LuceneTextIndexCreator creator = new LuceneTextIndexCreator(COLUMN, INDEX_DIR, true, false, null, null,
+ config)) {
+ for (String doc : DOCS) {
+ creator.add(doc);
+ }
+ creator.seal();
+ }
+
+ // LuceneTextIndexCreator.seal() force-merges to one segment, so the fixture above can never exercise
+ // cross-leaf accumulation. Build a second index by hand, committing between batches and never merging,
+ // so leaves() > 1.
+ FileUtils.forceMkdir(MULTI_LEAF_DIR);
+ try (Directory directory = FSDirectory.open(MULTI_LEAF_DIR.toPath());
+ IndexWriter writer = new IndexWriter(directory, new IndexWriterConfig(new StandardAnalyzer()))) {
+ for (String doc : DOCS) {
+ Document document = new Document();
+ document.add(new TextField(COLUMN, doc, Field.Store.NO));
+ writer.addDocument(document);
+ writer.commit();
+ }
+ }
+ }
+
+ @AfterClass
+ public void tearDown()
+ throws IOException {
+ FileUtils.deleteDirectory(TEMP_DIR);
+ }
+
+ @DataProvider(name = "queries")
+ public Object[][] queries() {
+ return new Object[][]{
+ {"order", null},
+ {"connection", null},
+ {"housekeeping", null},
+ {"nonexistentterm", null},
+ {"sentinelall", null},
+ {"to", null},
+ {"failed AND order", null},
+ {"failed OR connection", null},
+ {"order AND NOT failed", null},
+ {"\"place order\"", null},
+ {"\"failed to place order\"", null},
+ {"conn*", null},
+ {"c?che", null},
+ {"connection~1", null},
+ {"/conn.*/", null},
+ {"failed order connection", "parser=MATCH,minimumShouldMatch=2"},
+ {"*tion", "allowLeadingWildcard=true"}
+ };
+ }
+
+ /// The count must equal what materializing the doc ids would produce, for every query shape -- both the
+ /// shapes Lucene counts from metadata and the ones it has to iterate.
+ @Test(dataProvider = "queries")
+ public void testCountMatchesMaterializedDocIds(String query, String options)
+ throws IOException {
+ try (LuceneTextIndexReader reader = new LuceneTextIndexReader(COLUMN, INDEX_DIR, DOCS.length, Map.of())) {
Review Comment:
This fixture always ends up with an identity Lucene→Pinot mapping (`LuceneTextIndexCreator` stores `DocID == i`, so `TryOptimize` collapses to `NoOpDocIdTranslator`), which means the invariant the whole change rests on — that counts survive translation — is never tested against a real translator. A realtime conversion with a sorted column gives a genuine permutation via `mutableToImmutableDocIdMap`, and passing a `numDocs` smaller than the index would cover the `numDocs() > _numDocs` fallback that nothing exercises today. Worth adding both?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Count TEXT_MATCH filters without materializing matching doc ids [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 merged PR #19760:
URL: https://github.com/apache/pinot/pull/19760
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org