yashmayya opened a new issue, #19228:
URL: https://github.com/apache/pinot/issues/19228
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.
--
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]
