rhodo commented on code in PR #17817:
URL: https://github.com/apache/pinot/pull/17817#discussion_r2891994785
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -321,9 +324,11 @@ private class ColumnValueReader implements AutoCloseable {
}
private ForwardIndexReaderContext getReaderContext() {
- // Create reader context lazily to reduce the duration of existence
if (!_readerContextCreated) {
_readerContext = _reader.createContext();
Review Comment:
> There are a lot of places where reader.createContext() is invoked, e.g. in
ScanDocIdIterator. Do we need to also change that?
I did take a look at callsites broadly in the repo. Many places where
`_reader.createContext()` is invoked are segment-level operations without query
options (index rebuilding, segment loading, etc.), (though ScanDocIdIterator is
indeed a query-path call site), so for now I left it on an on-demand basis:
e.g. if later we need query options in ScanDocIdIterator we can propagate there
then.
Yeah I think having both `createContext()` and `createContext(queryOptions)`
with a default fallback works too, eventually no-arg `createContext()` are here
for the segment-level call sites. Happy to do this in a follow-up PR if you
think that's a cleaner approach.
--
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]
