Jackie-Jiang commented on code in PR #19273:
URL: https://github.com/apache/pinot/pull/19273#discussion_r3798022938
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/MutableOffHeapByteArrayStore.java:
##########
@@ -90,6 +90,14 @@ private static class Buffer implements Closeable {
private int _numValues = 0;
private int _availEndOffset; // Exclusive
+ /// Read-only view of the whole region, so that [#getByteBuffer] can hand
out a value view with a single `slice`
+ /// rather than the `duplicate` + `slice` + `asReadOnlyBuffer` chain
building one from scratch costs. Reads on the
+ /// consuming MAP path go through here once per row per projected key, so
those allocations add up.
+ ///
+ /// Built on first use rather than up front, so a store that is never read
this way - every dictionary and every
+ /// non-MAP raw column - behaves exactly as before. Racing readers may
each build one; they are interchangeable,
+ /// and the field is volatile so a reader never sees a half-initialized
buffer.
+ private volatile ByteBuffer _readOnlyView;
Review Comment:
This is a standalone optimization. Suggest separating it out as a separate
PR. Same for other optimizations around bytes read
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -97,9 +106,22 @@ public void addDataSource(String column, DataSource
dataSource) {
// RAW + shared-dict column must read raw values and consult the
dictionary directly.
Dictionary dictionary = forwardIndexReader.isDictionaryEncoded() ?
dataSource.getDictionary() : null;
ColumnValueReader columnValueReader = new
ColumnValueReader(forwardIndexReader, dictionary);
+ if (forwardIndexReader instanceof MapKeyIndexReader) {
+ MapKeyIndexReader mapKeyIndexReader = (MapKeyIndexReader)
forwardIndexReader;
+ MapKeyGroupReader groupReader =
_mapKeyGroupReaders.computeIfAbsent(mapKeyIndexReader.getForwardIndexReader(),
+ MapKeyGroupReader::new);
+ columnValueReader._mapKeyGroupReader = groupReader;
+ columnValueReader._mapKeyIndex = groupReader.addKey(mapKeyIndexReader);
+ }
_columnValueReaderMap.put(column, columnValueReader);
}
+ /// Marks the start of a block. [DataBlockCache] calls this before fetching
anything for the block; grouped MAP key
+ /// readers use it to tell one block's cached values from the next.
+ public void initNewBlock() {
Review Comment:
This is wrong coupling. Fetcher shouldn't know anything about caching. This
logic should be wrapped within the `DataBlockCache`, and fetcher is only
responsible of reading values once
--
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]
