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