arunkumarucet commented on PR #19865: URL: https://github.com/apache/pinot/pull/19865#issuecomment-6185881800
Thanks both — reworked in 8e8addc to the query-scoped design. Taking the points in turn. **Move the cache into `QueryContext` (@Jackie-Jiang).** Agreed, and your point about the benchmark was the deciding one. Per request the parse count is 250 (stock), 1 (query-scoped), ~0 (process-wide), so query-scoped was always going to capture essentially all of it, and the repeated-query benchmark could not show the difference. I measured the cold case you asked for — 100 unique, never-seen query strings per round, interleaving the two builds so machine drift hits both arms: | | stock | query-scoped | |---|---|---| | unique **term** queries | 2069.8 / 2033.8 ms | 2068.0 / 1938.0 ms | | unique **regexp** queries | 1638.7 / 1635.4 ms | **421.9 / 310.4 ms** | Term parses are too cheap to see against ~20 ms of query work; regexp parses build an automaton and dominate, so collapsing 250 of them to one is worth 4-5x **on the first execution**. A process-wide cache can only remove the one parse that remains, so it was not buying what the original numbers implied. Repeated-query results are unchanged by the rework (Q16 2.38x, Q17 1.68x, Q27 1.57x). Worth flagging on method: two earlier single-shot cold measurements disagreed by up to 47% in both directions. Only the interleaved runs are trustworthy — machine drift over a long session dwarfed the effect. **[P1] Structural key.** Real bug, fixed. The key is now a record whose components are immutable copies, so list equality is structural. I confirmed the failure mode before fixing it: with the flattened key, `["failed", "order"]` and `["failed, order"]` collide and the second config reuses the first's *empty* query, silently losing matches. `testStopWordListsThatRenderIdenticallyDoNotCollide` fails against a deliberately flattened key and passes with the record. **[P2] Atomic misses.** Fixed by construction — the contract is now `computeIfAbsent`. `testConcurrentMissesParseOnce` starts 16 threads on a cold key behind a latch and asserts the parse ran exactly once. **Plumbing.** `QueryScopedCache` is a 1-method interface in `pinot-segment-spi`; `QueryContext` implements it, so `pinot-segment-local` has no dependency on core. `TextIndexReader` gains two defaults that ignore the cache, and a `null` cache behaves exactly as the two-argument form, so other implementations and callers are unaffected. **@xiangfu0** — thanks for tracing the sharing-safety argument independently. Your three minor notes: - the deliberate-race note is moot now that misses go through `computeIfAbsent`; - the `"null"` options collision is gone with the structured key, since `optionsString` is a nullable component rather than concatenated text; - the realtime/multi-column gap is real; `RealtimeLuceneTextIndex` and `MultiColumnLuceneTextIndexReader` keep their uncached parse path, and I've called that out under "Not covered" in the description. Happy to file a follow-up. -- 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]
