yashmayya opened a new pull request, #19210:
URL: https://github.com/apache/pinot/pull/19210
## Problem
A lookup join reads the dimension table by primary key. The dimension table
is a hash map, and the map key is an array
of the primary key values. `LookupJoinOperator` built that array from the
equi-join keys, in the order that the join
condition lists them. The array matched the map key only by accident.
Six query shapes gave no rows or wrong rows because of this. None of them
gave an error.
Fixes #19188.
## The faults
Every case below is broken on the legacy planner and with
`usePhysicalOptimizer=true`. The dimension table primary key
is `[currency, rate_start_date]`.
| Join condition | Result before this change |
| --- | --- |
| `dim.currency = 'gbp' AND dim.rate_start_date = fact.rate_start_date` | 0
rows |
| The same condition written in `WHERE` | 0 rows |
| The same condition with `LEFT JOIN` | Every row is null-padded. Wrong
values, not missing rows. |
| `dim.rate_start_date = fact.rate_start_date AND dim.currency =
fact.currency` | 0 rows. Only the order differs from a condition that works. |
| `... AND dim.rate = fact.amount`, where `rate` is not a primary key column
| 0 rows |
| `dim.rate_start_date = fact.rate_start_date` alone | 0 rows |
## Root cause
The map key has three requirements. The old code met none of them.
**Length.** Calcite reads `dim.currency = 'gbp'` as a non-equi condition,
not as an equi-join key. The operator built
the key from the equi-join keys alone, so a primary key column that a
literal supplies was absent. The key was shorter
than the map key, and every lookup missed. The literal ran as a filter after
the lookup, so it never had an effect.
**Order.** The key positions followed the order of the join condition.
`rightKeys` names the dimension column of each
equi-join key, but the operator never read it. Two conditions in the other
order gave two key values in the wrong
places.
**Completeness.** A join key on a column outside the primary key has no
position in the key. The old code counted it
in the key length, so the key was too long and missed every row. A fix that
only skips such a key is worse. The
condition is absent from the non-equi conditions, so no filter applies it.
The join then returns rows that do not
match the condition, which is why this pull request raises an error instead.
## The fix
The constructor now compiles a key plan. The plan holds one entry per
primary key column, in the order that the
dimension table schema declares them. Two passes fill the plan.
1. **Equi-join keys.** `rightKeys[i]` names a dimension column. The position
of that column in the primary key decides
where `leftKeys[i]` lands. The key no longer depends on the order of the
join condition.
2. **Constants.** A non-equi condition of the form `dim_column = literal`
fills a position that pass 1 left open.
A constant never replaces an equi-join key. The equi-join key is held
nowhere else, so a replacement drops a join
condition and adds wrong rows. A constant on a position that pass 1 filled
stays a filter that runs after the lookup,
which is what SQL requires.
The operator converts each constant to the stored type of its column. A
literal already holds the internal value of
Pinot, but its numeric width follows the type that the planner gave the
literal. `PrimaryKey` compares values with
`equals`, where an `Integer` never equals a `Long`.
A constant that is null makes every lookup miss, because a null never
matches a primary key value.
A constant on a `BIG_DECIMAL` or `BYTES` primary key column is rejected.
`BigDecimal` compares its scale, so a literal
of `1.5` never matches a stored `1.50`. A `BYTES` literal is a `ByteArray`
while the dimension table stores `byte[]`,
whose `equals` is identity. Both give a key that misses every row, so an
error is better than an empty result.
SEMI and ANTI lookup joins project the left columns only, so a filter over
the join result cannot read a dimension
column. The operator now rejects a non-equi condition for those two join
types. The old code failed with an index
error from inside the filter instead.
## Errors in place of empty results
The key plan rejects a join condition that cannot give exactly one value per
primary key column:
- a join key on a dimension column outside the primary key
- more than one join key on the same primary key column
- a primary key column that no condition fills
Each of these gave no rows or wrong rows before this change, so an error is
the better outcome. The error names the
columns and tells the user to add the missing conditions or to remove the
lookup join hint. This is the contract that
the single-stage `lookup` transform function already enforces.
This is a behavior change. A query that returns 0 rows in silence today can
now return an error. A query that returns
correct rows today is not affected.
## Test harness
Two faults in `ResourceBasedQueriesTest` kept this area out of reach of
tests.
1. The mock `DimensionTableDataManager` did not stub
`getPrimaryKeyColumns()`. Mockito returned an empty list, so any
fix that reads the primary key gets a key of length 0 and breaks every
lookup join test.
2. The mock built its map from the raw values of the test case JSON, with no
conversion to the type of the column. The
map held an `Integer` where the query supplied a `Long`, so a lookup
missed for a reason outside the code under
test. A test with a `LONG` primary key column cannot pass.
This pull request corrects both.
## Tests
`LookupJoin.json` runs end to end on the legacy planner and with
`usePhysicalOptimizer=true`:
- the query from issue #19188
- a literal primary key component in `ON`, and the same condition in `WHERE`
- a literal primary key component with `LEFT JOIN`, including a fact row
with no match
- both primary key columns equi-joined, in primary key order and in reverse
primary key order
- a literal on a primary key column that an equi-join key already fills
- a literal that fills the `LONG` primary key column
- an open primary key column, a set predicate such as `IN`, and a join key
outside the primary key
`LookupJoinOperatorTest` tests the key plan directly, for the cases that a
query cannot reach:
- a null literal, which makes every lookup miss
- every branch of the stored type conversion, over a data provider,
asserting the exact runtime class
- a constant on a `BIG_DECIMAL` or `BYTES` column, and a non-numeric
constant on a numeric column
- two join keys on the same primary key column
- a dimension table with no primary key columns
I ran each new test against the fault that it targets, and each one fails
without the fix.
## Related pull requests
This supersedes #19200.
--
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]
