apache / apache/pinot

BYTES primary key columns in dimension tables can never be looked up

Open
#19,228 0 comments 0 reactions 0 assignees View on GitHub
bug query
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.