xiangfu0 commented on code in PR #19273:
URL: https://github.com/apache/pinot/pull/19273#discussion_r3810465568
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/MapKeyIndexReader.java:
##########
@@ -33,12 +33,29 @@ public class MapKeyIndexReader implements
ForwardIndexReader {
private final FieldSpec _keyFieldSpec;
private final PreparedMapKey _mapKey;
private final Object _defaultNullValue;
+ private final String _defaultNullValueString;
public MapKeyIndexReader(ForwardIndexReader forwardIndexReader, String
keyName, FieldSpec keyFieldSpec) {
_forwardIndexReader = forwardIndexReader;
_mapKey = new PreparedMapKey(keyName);
_keyFieldSpec = keyFieldSpec;
_defaultNullValue = keyFieldSpec.getDefaultNullValue();
+ _defaultNullValueString = _defaultNullValue.toString();
+ }
Review Comment:
Good catch, fixed. `FieldSpec#getDefaultNullValueString` now supplies it.
The two agree for every scalar type except BYTES, whose default rendered as an
identity string (`[B@1b6d3586`) instead of hex — `getString` on master had the
same `toString()` call inline, so this fixes a pre-existing bug rather than one
the caching introduced.
##########
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:
Fair — the PR is doing more than one thing. Flagging that the split is not
in this push: the author asked for the branch squashed to a single commit, so
it is currently one commit containing the sealed-segment selective read, the
byte-read optimizations you pointed at, and the multi-key grouping.
Happy to carve it into three: (A) the sealed selective read, which is the
actual defect — sealed MAP columns had no selective path at all and
deserialized the whole frame per key per document; (B) the byte-read work here
(shared off-heap view, `JsonBuilder` bulk copy, word-at-a-time key compare);
(C) the multi-key grouping. A is the smallest and carries nearly all of the
win, so it would go first. Let me know if you would rather review it that way
and I will split it.
##########
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:
Agreed, and the result is smaller than what it replaced. All caching is out
of `DataFetcher`: the `initNewBlock()`/`_blockId` stamping and the per-block
value cache inside `MapKeyGroupReader` are gone.
`DataFetcher` now only groups the key readers that share a forward index and
exposes two read-only methods — `getMapKeyGroupColumns(column)` and
`fetchStringValuesForMapKeyGroup(...)`.
`DataBlockCache#getStringValuesForSVColumn` decides when a block's values are
still good, reusing the `markLoaded`/`getValues` machinery it already has for
every other column: it fills each sibling's block buffer and marks them loaded,
so their own lookups are served from there. That also drops my parallel
block-invalidation logic, which was duplicating what `initNewBlock` already
does.
--
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]
