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: [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]
