BYTES primary key columns in dimension tables can never be looked up
- Dominant language
- Java
- Stars
- 6.1k
- Forks
- 1.5k
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 189
Description
A dimension table with a `BYTES` primary key column can never be looked up. Every probe misses, whatever the caller
passes. This affects the single-stage `lookup` transform function and the multi-stage lookup join, in both dimension
table storage modes.
The failure is silent. `containsKey` returns false, `lookupRow` / `lookupValue` / `lookupValues` return null, and the
query returns no rows with no error.
## Reproduction
I built a real single-column `BYTES` primary key dimension table from a real segment, loaded it through
`DimensionTableDataManager`, and probed it four ways. A `STRING` primary key is included as a control.
```
########## PK TYPE = BYTES disablePreload=false ##########
stored key class = [B
probe with ByteArray(copy) -> containsKey=false lookupValues=null
probe with ByteArray(same instance) -> containsKey=false lookupValues=null
probe with byte[] copy -> containsKey=false lookupValues=null
probe with byte[] same instance -> containsKey=false lookupValues=null
########## PK TYPE = STRING disablePreload=false ##########
stored key class = java.lang.String
probe with String -> containsKey=true lookupValues=[abc, alpha]
########## PK TYPE = BYTES disablePreload=true ##########
stored key class = [B
probe with ByteArray(copy) -> containsKey=false lookupValues=null
probe with ByteArray(same instance) -> containsKey=false lookupValues=null
probe with byte[] copy -> containsKey=false lookupValues=null
probe with byte[] same instance -> containsKey=false lookupValues=null
########## PK TYPE = STRING disablePreload=true ##########
stored key class = java.lang.String
probe with String -> containsKey=true lookupValues=[abc, alpha]
```
Note that even the identical `byte[]` instance that was ingested fails to match, because the value in the map is a new
array produced when the segment was read back.
## Root cause
The lookup map stores the primary key as an `Object[]` whose `BYTES` element is a raw `byte[]`:
1. `DimensionTableDataManager` builds each key with `recordReader.getRecordValues(i, pkIndexes)`
([line 242](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java#L242)
for the preloaded table, [line 320](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java#L320)
for the memory-optimized one).
2. That reaches `PinotSegmentColumnReader.getValue`, which returns a raw `byte[]` for a `BYTES` column on both the
dictionary-encoded path (`BytesDictionary.get` returns `byte[]`, while the `ByteArray` variant is `getInternal`,
which this path does not call) and the raw path (`ForwardIndexReader.getBytes`).
3. The map compares keys with `DimensionTableDataManager.HASH_STRATEGY`
([line 64](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java#L64)),
which is `Arrays.hashCode(Object[])` and `Arrays.equals(Object[])`. Both delegate to the `hashCode` and `equals` of
each element.
4. `byte[]` inherits both from `Object`, so they are identity based.
A key that compares by identity cannot be reproduced by a caller. A `ByteArray` fails because it is a different class
with a content based hash, and a fresh `byte[]` fails because it is a different object.
## Callers
`LookupTransformFunction` wraps the probe value in a `ByteArray`
([line 218](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LookupTransformFunction.java#L218)),
which is the representation the rest of Pinot uses for `BYTES`. That is the reasonable choice, and it still misses,
because the stored side is the part that is wrong.
The multi-stage lookup join reaches the same map through `lookupValues` and misses the same way.
## Why no test caught it
`LookupTransformFunctionTest.primaryKeyTypeTest`
([line 346](https://github.com/apache/pinot/blob/master/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/LookupTransformFunctionTest.java#L346))
covers a `BYTES` primary key, but it mocks `DimensionTableDataManager` and stubs `lookupValue` to return
`"lookup_value_for_[" + pk.hashCode() + "]"`. It asserts the hash of the key that the transform function builds and
never loads a dimension table. It therefore verifies that the probe is well formed, and cannot observe whether the
probe matches anything.
## Suggested fix
Store `ByteArray` rather than `byte[]` in the key array, in both `createFastLookupDimensionTable` and the
memory-optimized path. `LookupTransformFunction` already sends a `ByteArray`, so it starts working with no change.
The multi-stage lookup join then works as well.
Worth adding at the same time:
- A test that loads a real dimension table with a `BYTES` primary key and asserts a successful lookup. The current test
cannot fail, whatever the storage side does.
- The same check for `UUID`, which shares the `BYTES` stored type.
Until this is fixed, PR #19210 rejects a `BYTES` primary key constant in the multi-stage lookup join with a clear
error, rather than returning an empty result.
## Related
`BIG_DECIMAL` primary keys have a milder version of the same class of problem. `BigDecimal.equals` compares the scale,
so a stored `1.50` does not match a probe of `1.5`. That one is not specific to dimension tables, because a hash join
compares `BIG_DECIMAL` keys the same way through `ObjectLookupTable`, so it is left out of this issue.
Contributor guide
Research direction
Start in DimensionTableDataManager at createFastLookupDimensionTable and the memory-optimized path, where primary-key arrays are built, then inspect HASH_STRATEGY and the BYTES values returned by PinotSegmentColumnReader. Add coverage that loads a real BYTES primary-key dimension table and verifies lookup success in both storage modes; include UUID if feasible. Confirm the existing LookupTransformFunctionTest coverage is complemented by a non-mocked lookup test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100