Jackie-Jiang opened a new pull request, #19634:
URL: https://github.com/apache/pinot/pull/19634
## Summary
The batch murmur3 hash reads (`read32BitsMurmur3HashValues` /
`read64BitsMurmur3HashValues` / `read128BitsMurmur3HashValues`) on immutable
`BytesDictionary` and `BigDecimalDictionary` hash truncated values when the
dictionary uses the fixed-byte layout.
The reads went through the `ValueReader` hash defaults, which hash
`readUnpaddedBytes`. `FixedByteValueReaderWriter.readUnpaddedBytes` stops at
the first `0x00` byte because the fixed-byte layout pads STRING values with
`\0`. For BYTES and BIG_DECIMAL,
`DictionaryIndexType.shouldUseVarLengthDictionary` picks the fixed-byte layout
only when all values have the same length, so there is no padding and `0x00` is
real data:
- A BIG_DECIMAL with scale 0 serializes with leading `0x00` scale bytes, so
every such value hashed as the empty array.
- A BYTES value hashed only its bytes before the first `0x00`, so e.g. `{1,
0, 0, 0}` and `{1, 0, 0, 1}` collided.
- The batch hash disagreed with the single-value
`Dictionary#get*BitsMurmur3HashValue`, which hashes the full `getBytesValue`.
Changes:
- Add `ValueReader#readBytes`, which reads the full value (the whole slot
for fixed-byte, the exact length for var-length), next to `readUnpaddedBytes`,
which is now documented as STRING-only.
- Hash in the dictionaries from the read that matches the type: `readBytes`
for BYTES / BIG_DECIMAL, `readUnpaddedBytes` for STRING. STRING hashes are
unchanged. The per-row loops reuse one scratch buffer and stay branch-free, and
the fixed-byte read is now a single bulk copy instead of a word-by-word zero
scan.
- Remove the padding-aware hash defaults from `ValueReader` and the matching
public helpers from `BaseImmutableDictionary`, so the STRING padding semantics
can no longer be applied to other types by accident.
- `ImmutableDictionaryTest` adds fixed-byte BYTES (leading, interior and
trailing `0x00`) and scale-0 BIG_DECIMAL dictionaries, asserting the batch
hashes match the single-value hashes and are distinct, and checks batch against
single-value hashes for the existing STRING, BIG_DECIMAL and BYTES dictionaries.
## Reachability
No query returns wrong results today, so this fixes a latent inconsistency:
- The only production consumer of `BlockValSet#get*BitsMurmur3HashValuesSV`
is `DistinctCountOffHeapAggregationFunction`, which takes the hash path only
when `blockValSet.isDictionaryEncoded()` is false. For dictionary-encoded input
it collects dict ids and later hashes through the single-value
`Dictionary#get*BitsMurmur3HashValue`, which is correct.
- `ProjectionBlockValSet#isDictionaryEncoded` and
`DataFetcher#addDataSource` use the same condition (a dictionary plus a
dict-encoded forward index), so when the hash path runs, `DataFetcher` has no
dictionary and reads hashes from the forward index. The dictionary branch in
`DataFetcher.ColumnValueReader#read*BitsMurmur3HashValues` is not reached.
- `StarTreePreAggregatedBlockValSet` delegates `isDictionaryEncoded`, and
`TransformBlockValSet` hashes `getBytesValuesSV`, which reads the full value.
## Compatibility
No on-disk or wire format changes. `ValueReader` (in `pinot-segment-local`,
not an SPI) gains the abstract `readBytes` and loses the three hash defaults,
and `BaseImmutableDictionary` loses its public `get32BitsMurmur3Hash(int,
byte[])` / `get64BitsMurmur3Hash(int, byte[])` /
`get128BitsMurmur3HashValue(int, byte[])` helpers. `FixedByteValueReaderWriter`
and `VarLengthValueReader` are the only `ValueReader` implementations.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
