Build the lookup join key from the dimension table primary key by yashmayya · Pull Request #19210 · apache/pinot

yashmayya · 2026-08-10T22:58:12Z

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.

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.
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 does not convert a constant. The planner coerces the operands of a comparison, so a constant compared
against a dimension column already carries the type of that column. The operator checks that this holds, and names the
table and column if it ever stops. PrimaryKey compares values with equals, where an Integer never equals a
Long, so a constant of another type misses every row.

A constant that is null makes every lookup miss, because a null never matches a primary key value.

A constant on a BYTES primary key column is rejected. A dimension table reads its key values as a raw byte[]. The
equals and hashCode of a byte[] are identity, so no constant of any representation can match one. The
single-stage lookup transform function has the same limit. The fix for both belongs in DimensionTableDataManager.

A constant on a BIG_DECIMAL primary key column is allowed. BigDecimal compares its scale, so 1.5 does not match a
stored 1.50. A hash join carries the same hazard, and this operator does not single out one arm of it.

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.

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.
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 Lookup Join returns 0 rows when given a literal value #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 constant, which makes every lookup miss
a constant of the type of its column, and a constant of another type, which the operator rejects
a constant on a BIG_DECIMAL column, which is allowed, and on a BYTES column, which is rejected
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.

The lookup key was built from the equi-join keys in join-condition order, so it silently returned no rows when a primary key column was bound by a literal, when the conditions were written in a different order from the primary key, or when a join key was on a non-primary-key column. Fixes apache#19188

codecov-commenter · 2026-08-10T23:43:50Z

Codecov Report

❌ Patch coverage is 79.26829% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.94%. Comparing base (a2f96c4) to head (e56a153).
⚠️ Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
...not/query/runtime/operator/LookupJoinOperator.java	79.26%	7 Missing and 10 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #19210       +/-   ##
=============================================
+ Coverage     38.95%   66.94%   +27.99%     
- Complexity     1422     1423        +1     
=============================================
  Files          3452     3452               
  Lines        218708   218631       -77     
  Branches      34789    34750       -39     
=============================================
+ Hits          85196   146372    +61176     
+ Misses       125746    60558    -65188     
- Partials       7766    11701     +3935

Flag	Coverage Δ
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-25	`66.94% <79.26%> (+27.99%)`	⬆️
lane-a	`100.00% <ø> (ø)`
lane-b	`0.00% <ø> (ø)`
temurin	`66.94% <79.26%> (+27.99%)`	⬆️
unittests	`66.94% <79.26%> (+27.99%)`	⬆️
unittests1	`57.70% <79.26%> (?)`
unittests2	`39.01% <0.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

timothy-e

Thanks for getting on this so fast! I have a few questions, but the approach looks awesome.

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.

Could we improve perf by following up with a change that allows us to apploy both filters during the hashmap lookup, to avoid materializing rows just to filter them out later?

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.

From my understanding, this kind of scenario could have occured before if we joined a Int column with a Long column, but it was avoided because the planner added casts. Could we do the same thing here, and put the casting in the planner instead of in the operator? (It seems a little messy to me to add a toStoredValue because I feel like that logic must have been implemented somewhere else already)

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

Both of these seem like they could be added as filters after the join, which would increase our SQL compatibility?

timothy-e · 2026-08-11T14:42:29Z

+  /// stored `1.50`. BYTES is rejected because the literal is a [org.apache.pinot.spi.utils.ByteArray] while the
+  /// dimension table stores `byte[]`, whose `equals` is identity.


the SSE lookup join wraps byte[] in a ByteArray to compare, can we do the same thing here? https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LookupTransformFunction.java#L218

Good point, but I think wrapping won't help and BYTES dimension primary keys are probably broken today, everywhere.

The dimension table's map key holds a raw byte[]: PinotSegmentRecordReader.getRecordValues → PinotSegmentColumnReader.getValue → either BytesDictionary.get() (returns byte[], while the ByteArray-returning variant is getInternal, which is not called here) or ForwardIndexReader.getBytes(). And DimensionTableDataManager.HASH_STRATEGY is Arrays.hashCode/Arrays.equals over Object[], which delegate to each element's own hashCode/equals - identity for byte[].

So nothing can ever probe a BYTES primary key: not a ByteArray (different class, content-based hash), not even a fresh byte[] with the same bytes. That includes LookupTransformFunction itself - so I don't think the code you've linked is a working reference.

Let's track this separately since it doesn't really relate to the fix here.

makes sense, thanks for the explanation!

timothy-e · 2026-08-11T14:49:47Z

+  /// match the stored representation misses every row, and this operator reports no rows the same way whether the key
+  /// is genuinely absent or malformed. The single-stage `lookup` transform function rejects the same way.
+  ///
+  /// BIG_DECIMAL is rejected because `BigDecimal#equals` compares the scale, so a literal of `1.5` never matches a


How does BIG_DECIMAL work on other join types?

I think the same hazard exists elsewhere too TBH, so this isn't lookup join specific. I've updated this.

The planner coerces the operands of a comparison, so a constant compared against a dimension column already carries that column's type. Replace the conversion with a check that states the assumption. Allow BIG_DECIMAL, whose scale sensitivity a hash join shares, and keep rejecting BYTES, which a dimension table keys on a raw byte[] that nothing can match. Name the column ids in the operator test, as suggested in review.

yashmayya

Thanks for the thorough review @timothy-e!

Could we improve perf by following up with a change that allows us to apploy both filters during the hashmap lookup, to avoid materializing rows just to filter them out later?

No row is materialized and discarded: JoinedRowView is a lazy view over the two rows and toArray() runs only after the filters pass (there is a // defer copying of the content until row matches comment); see #17542. The waste is the evaluator call plus the probe, not a row copy.

it was avoided because the planner added casts

Yes, fair point, I wasn't really able to find any cases where the planner wasn't already adding the right casts at the logical plan level. I've removed the redundant toStoredValue conversion.

Both of these seem like they could be added as filters after the join, which would increase our SQL compatibility

Agreed, but I'd rather add support for these as follow ups rather than bloating this bug fix PR further.

yashmayya · 2026-08-11T21:37:09Z

+  /// stored `1.50`. BYTES is rejected because the literal is a [org.apache.pinot.spi.utils.ByteArray] while the
+  /// dimension table stores `byte[]`, whose `equals` is identity.


Good point, but I think wrapping won't help and BYTES dimension primary keys are probably broken today, everywhere.

The dimension table's map key holds a raw byte[]: PinotSegmentRecordReader.getRecordValues → PinotSegmentColumnReader.getValue → either BytesDictionary.get() (returns byte[], while the ByteArray-returning variant is getInternal, which is not called here) or ForwardIndexReader.getBytes(). And DimensionTableDataManager.HASH_STRATEGY is Arrays.hashCode/Arrays.equals over Object[], which delegate to each element's own hashCode/equals - identity for byte[].

So nothing can ever probe a BYTES primary key: not a ByteArray (different class, content-based hash), not even a fresh byte[] with the same bytes. That includes LookupTransformFunction itself - so I don't think the code you've linked is a working reference.

Let's track this separately since it doesn't really relate to the fix here.

yashmayya · 2026-08-11T21:40:12Z

+  /// match the stored representation misses every row, and this operator reports no rows the same way whether the key
+  /// is genuinely absent or malformed. The single-stage `lookup` transform function rejects the same way.
+  ///
+  /// BIG_DECIMAL is rejected because `BigDecimal#equals` compares the scale, so a literal of `1.5` never matches a


I think the same hazard exists elsewhere too TBH, so this isn't lookup join specific. I've updated this.

timothy-e

Nice, thanks Yash!

Both of these seem like they could be added as filters after the join, which would increase our SQL compatibility

Agreed, but I'd rather add support for these as follow ups rather than bloating this bug fix PR further.

+1, just thinking out loud

Copilot

Pull request overview

Fixes lookup joins by constructing dimension-table lookup keys in primary-key order, including literal-bound key components.

Changes:

Compiles and validates lookup-key plans.
Improves dimension-table test mocks and type handling.
Adds unit and end-to-end regression coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`LookupJoinOperator.java`	Builds and validates primary-key lookup plans.
`LookupJoinOperatorTest.java`	Tests key-plan ordering, constants, and validation.
`ResourceBasedQueriesTest.java`	Enhances dimension-table mocks.
`LookupJoin.json`	Adds composite-key regression scenarios.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    switch (fieldSpec.getDataType().getStoredType()) {
+      case INT:
+        return ((Number) value).intValue();
+      case LONG:
+        return ((Number) value).longValue();
+      case FLOAT:
+        return ((Number) value).floatValue();
+      case DOUBLE:
+        return ((Number) value).doubleValue();
+      case STRING:
+        return value.toString();
+      default:
+        return value;
+    }


+        "ignoreLiteMode": true
+      },
+      {
+        "description": "A literal binds the LONG primary-key column. The planner types the literal, and a literal of the wrong numeric width misses every row, so the operator converts it to the stored type of the column.",


yashmayya · 2026-08-12T22:46:20Z

+    // SEMI and ANTI joins project the left columns only, so an evaluator built over the join result schema cannot
+    // reference a dimension table column. Reject the combination here, otherwise the loop below fails with an index
+    // error that says nothing about the cause.
+    Preconditions.checkState(nonEquiConditions.isEmpty() || _joinType.projectsRight(),
+        "Lookup join type: %s does not support non-equi join conditions, got: %s", _joinType, nonEquiConditions);


Seems redundant to add a test for a clearly unsupported path.

…escription Reading the segment with PinotSegmentRecordReader is what a real DimensionTableDataManager does, so the mock no longer keeps its own partial copy of the stored-type conversion rules.

yashmayya mentioned this pull request Aug 10, 2026

fix: incorporate literal values into lookup join key #19200

Open

yashmayya added bug Something is not working as expected multi-stage Related to the multi-stage query engine labels Aug 10, 2026

yashmayya requested review from Jackie-Jiang, gortiz and xiangfu0 August 10, 2026 23:26

timothy-e reviewed Aug 11, 2026

View reviewed changes

yashmayya commented Aug 11, 2026

View reviewed changes

timothy-e approved these changes Aug 12, 2026

View reviewed changes

This was referenced Aug 12, 2026

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

Open

Lookup join: fold constants out of the post-lookup filter, and accept join conditions a filter can satisfy #19229

Open

Jackie-Jiang requested a balanced review from Copilot August 12, 2026 22:36

Copilot started reviewing on behalf of Jackie-Jiang August 12, 2026 22:37 View session

Copilot AI reviewed Aug 12, 2026

View reviewed changes

Build the mock dimension table from the segment, and correct a test d…

e56a153

…escription Reading the segment with PinotSegmentRecordReader is what a real DimensionTableDataManager does, so the mock no longer keeps its own partial copy of the stored-type conversion rules.

		/// stored `1.50`. BYTES is rejected because the literal is a [org.apache.pinot.spi.utils.ByteArray] while the
		/// dimension table stores `byte[]`, whose `equals` is identity.

Uh oh!

Conversation

yashmayya commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

The faults

Root cause

The fix

Errors in place of empty results

Test harness

Tests

Related pull requests

Uh oh!

codecov-commenter commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

timothy-e left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yashmayya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timothy-e left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yashmayya commented Aug 10, 2026 •

edited

Loading

codecov-commenter commented Aug 10, 2026 •

edited

Loading

timothy-e left a comment •

edited

Loading