arunkumarucet commented on code in PR #19865:
URL: https://github.com/apache/pinot/pull/19865#discussion_r4290871099
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/TextIndexReader.java:
##########
@@ -61,6 +61,23 @@ default int getNumMatchingDocs(String searchQuery, @Nullable
String optionsStrin
return getDocIds(searchQuery, optionsString).getCardinality();
}
+ /// Returns the matching document ids, reusing a query-scoped parse when the
implementation supports it.
+ ///
+ /// Parsing a search string depends only on the string and the column's
index configuration, so every
+ /// segment of one request would otherwise repeat identical work.
Implementations that parse should
+ /// derive their cache key from the search string, the options and the
configuration that affects
+ /// parsing. A `null` cache means "do not share", and must behave exactly
like the two-argument form.
+ default MutableRoaringBitmap getDocIds(String searchQuery, @Nullable String
optionsString,
Review Comment:
Fixed in `17dbe4df0d`. I reproduced the ambiguity first, then tested both
options you suggested:
| variant | `getDocIds("body", "error", null)` | `getDocIds(null, "error",
null)` |
|---|---|---|
| cache last (previous) | ambiguous | ambiguous |
| cache first | compiles | **still ambiguous** |
| distinct names | compiles | compiles |
Putting the cache first fixes the call you reported but not the general
case: `null` is still applicable to both `String column` and `QueryScopedCache`
on a multi-column reader, and neither candidate is more specific. So I went
with distinct names — `getDocIdsWithParseCache` and
`getNumMatchingDocsWithParseCache` — which removes the overload set entirely.
The name also states a contract the overload obscured: only the *parsed
query* is shared. Doc ids are per-segment and must never be cached across
readers.
Added `testMultiColumnNullOptionsCallsRemainUnambiguous`, which makes the
documented null-options calls. It asserts little at runtime; its job is to fail
compilation if this regresses. I checked it isn't vacuous by restoring the old
names, which reproduces both ambiguous-reference errors.
--
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]
