xiangfu0 commented on code in PR #19865:
URL: https://github.com/apache/pinot/pull/19865#discussion_r4276995090
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -297,9 +335,25 @@ public int getNumMatchingDocs(String searchQuery,
@Nullable String optionsString
/// Both [#getDocIds(String, String)] and [#getNumMatchingDocs] go through
this method, so a `count(*)` and
/// the filter it counts cannot interpret the same query differently. Keep
it that way: any new entry point
/// that parses a search string itself reintroduces that divergence.
+ /// Returns a cached instance when one is present. Callers must treat the
result as shared and
+ /// immutable, which Lucene queries are.
@VisibleForTesting
Query buildQuery(String searchQuery, @Nullable String optionsString)
throws Exception {
+ String cacheKey = _queryCacheScope + '\u0001' + searchQuery + '\u0001' +
optionsString;
+ Query cached = QUERY_CACHE.getIfPresent(cacheKey);
+ if (cached != null) {
+ return cached;
+ }
+ Query query = parseQuery(searchQuery, optionsString);
+ QUERY_CACHE.put(cacheKey, query);
Review Comment:
Minor: the `getIfPresent`/`put` pair is a benign check-then-act race — on a
cold key, every worker thread that misses concurrently parses and puts, last
put wins. That's harmless here (parses are side-effect-free and the results are
equivalent), and arguably better than `cache.get(key, loader)`'s per-key
locking on this hot path. Worth one sentence in the doc comment stating the
race is deliberate, so a future reader doesn't "fix" it into a lock.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -297,9 +335,25 @@ public int getNumMatchingDocs(String searchQuery,
@Nullable String optionsString
/// Both [#getDocIds(String, String)] and [#getNumMatchingDocs] go through
this method, so a `count(*)` and
/// the filter it counts cannot interpret the same query differently. Keep
it that way: any new entry point
/// that parses a search string itself reintroduces that divergence.
+ /// Returns a cached instance when one is present. Callers must treat the
result as shared and
+ /// immutable, which Lucene queries are.
@VisibleForTesting
Query buildQuery(String searchQuery, @Nullable String optionsString)
throws Exception {
+ String cacheKey = _queryCacheScope + '\u0001' + searchQuery + '\u0001' +
optionsString;
Review Comment:
Minor: string concatenation turns a null `optionsString` into `"null"`, so a
literal options string `"null"` shares a key with no-options. I checked that
this is harmless — `hasOptions("null")` parses to zero options, so both take
the identical no-options parse path — but `Objects.toString(optionsString,
"")`-style normalization (or a short comment) would make the key self-evidently
safe.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -79,6 +81,26 @@ public class LuceneTextIndexReader implements
TextIndexReader {
private final String _queryParserClass;
private Constructor<QueryParserBase> _queryParserClassConstructor;
private boolean _enablePrefixSuffixMatchingInPhraseQueries = false;
+ /// Identifies every piece of configuration that changes how a search string
parses, so that two
+ /// readers share a cached query only when they would have parsed it
identically. Computed once.
+ private final String _queryCacheScope;
+
+ /// Parsed queries, shared by every segment reader in this process.
Review Comment:
Note on coverage (fine as a follow-up): this cache only helps
immutable-segment readers. `RealtimeLuceneTextIndex` (consuming segments) and
`MultiColumnLuceneTextIndexReader` each have their own uncached parse path with
the same duplicate-parse shape, so realtime-heavy tables won't see the win yet.
Worth a line in the PR description and possibly a follow-up issue.
--
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]
