You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/08/10 22:58:14 UTC
[PR] Build the lookup join key from the dimension table primary key [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "timothy-e (via GitHub)" <gi...@apache.org>.
timothy-e commented on code in PR #19210:
URL: https://github.com/apache/pinot/pull/19210#discussion_r3766839562
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,194 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
_nonEquiEvaluators = new ArrayList<>(nonEquiConditions.size());
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition, _resultSchema));
}
+
+ KeyPlan keyPlan =
+ compileKeyPlan(node, rightTableName, _rightTable.getPrimaryKeyColumns(), _rightInput.getDataSchema(),
+ _leftColumnSize);
+ _keySize = keyPlan._sources.length;
+ _keySources = keyPlan._sources;
+ _keyConstants = keyPlan._constants;
+ _neverMatches = keyPlan._neverMatches;
+ }
+
+ /// Works out where each value of the lookup key comes from.
+ ///
+ /// The key has one position per dimension table primary key column, in the order the dimension table schema declares
+ /// them. Each position is bound in two passes:
+ ///
+ /// 1. Equi-join keys. `rightKeys[i]` names a dimension column, and that column's position in the primary key decides
+ /// where `leftKeys[i]` lands. This is what makes the key independent of the order of the join condition.
+ /// 2. Constants. A non-equi condition of the form `dim_column = literal` binds a position that pass 1 left open.
+ /// A constant never replaces an equi-join key, because the equi-join key is not kept anywhere else and dropping it
+ /// would silently widen the join. A constant that pass 1 already bound stays in [#_nonEquiEvaluators] and runs as
+ /// a filter after the lookup, which is what the SQL semantics require.
+ ///
+ /// The method rejects a join condition that cannot produce exactly one value per primary key column. Every rejected
+ /// case returned no rows or wrong rows before this validation existed, so an error is the better outcome. This is
+ /// also the contract that the single-stage `lookup` transform function enforces.
+ @VisibleForTesting
+ static KeyPlan compileKeyPlan(JoinNode node, String tableName, @Nullable List<String> primaryKeyColumns,
+ DataSchema rightSchema, int leftColumnSize) {
+ Preconditions.checkState(CollectionUtils.isNotEmpty(primaryKeyColumns),
+ "Failed to find primary key columns for dimension table: %s", tableName);
+ String[] rightColumns = rightSchema.getColumnNames();
+ int keySize = primaryKeyColumns.size();
+ int[] sources = new int[keySize];
+ Arrays.fill(sources, KEY_SOURCE_UNBOUND);
+ Object[] constants = new Object[keySize];
+
+ // Pass 1: bind key positions from the equi-join keys.
+ List<Integer> leftKeys = node.getLeftKeys();
+ List<Integer> rightKeys = node.getRightKeys();
+ int numEquiKeys = leftKeys.size();
+ for (int i = 0; i < numEquiKeys; i++) {
+ String rightColumn = rightColumns[rightKeys.get(i)];
+ int keyPosition = primaryKeyColumns.indexOf(rightColumn);
+ Preconditions.checkState(keyPosition >= 0,
+ "Lookup join on dimension table: %s has a join key on column: %s, which is not a primary key column. "
+ + "Primary key columns: %s", tableName, rightColumn, primaryKeyColumns);
+ Preconditions.checkState(sources[keyPosition] == KEY_SOURCE_UNBOUND,
+ "Lookup join on dimension table: %s has multiple join keys on primary key column: %s", tableName,
+ rightColumn);
+ sources[keyPosition] = leftKeys.get(i);
+ }
+
+ // Pass 2: bind the remaining key positions from constant equality conditions.
+ boolean neverMatches = false;
+ for (RexExpression nonEquiCondition : node.getNonEquiConditions()) {
+ int rightColumnId = getConstantEqualityColumnId(nonEquiCondition, leftColumnSize, rightColumns.length);
+ if (rightColumnId < 0) {
+ continue;
+ }
+ int keyPosition = primaryKeyColumns.indexOf(rightColumns[rightColumnId]);
+ if (keyPosition < 0 || sources[keyPosition] != KEY_SOURCE_UNBOUND) {
+ continue;
+ }
+ sources[keyPosition] = KEY_SOURCE_CONSTANT;
+ Object value = getConstantValue(nonEquiCondition);
+ constants[keyPosition] =
+ toStoredValue(value, rightSchema.getColumnDataType(rightColumnId), tableName, rightColumns[rightColumnId]);
+ neverMatches |= constants[keyPosition] == null;
+ }
+
+ List<String> unboundColumns = new ArrayList<>();
+ for (int i = 0; i < keySize; i++) {
+ if (sources[i] == KEY_SOURCE_UNBOUND) {
+ unboundColumns.add(primaryKeyColumns.get(i));
+ }
+ }
+ Preconditions.checkState(unboundColumns.isEmpty(),
+ "Lookup join on dimension table: %s cannot determine primary key columns: %s from the join condition. "
+ + "A lookup join reads the dimension table by primary key, so the join condition must have an equality on "
+ + "every primary key column: %s. Add the missing conditions, or remove the lookup join hint to use a hash "
+ + "join instead.", tableName, unboundColumns, primaryKeyColumns);
+ return new KeyPlan(sources, constants, neverMatches);
+ }
+
+ /// Returns the dimension table column id of a `dim_column = literal` condition, or -1 when the condition does not
+ /// have that shape. Only an equality against a single literal can serve as a key value. A condition such as
+ /// `dim_column IN ('a', 'b')` reaches this method as a disjunction and returns -1, because a hash lookup cannot read
+ /// a set of keys.
+ ///
+ /// Non-equi conditions index the joined row, so the dimension table columns start at `leftColumnSize`.
+ private static int getConstantEqualityColumnId(RexExpression condition, int leftColumnSize, int numRightColumns) {
+ if (!(condition instanceof RexExpression.FunctionCall)) {
+ return -1;
+ }
+ RexExpression.FunctionCall functionCall = (RexExpression.FunctionCall) condition;
+ if (!functionCall.getFunctionName().equals(SqlKind.EQUALS.name())) {
+ return -1;
+ }
+ List<RexExpression> operands = functionCall.getFunctionOperands();
+ if (operands.size() != 2) {
+ return -1;
+ }
+ RexExpression inputRef = operands.get(0) instanceof RexExpression.InputRef ? operands.get(0) : operands.get(1);
+ RexExpression literal = operands.get(0) instanceof RexExpression.InputRef ? operands.get(1) : operands.get(0);
+ if (!(inputRef instanceof RexExpression.InputRef) || !(literal instanceof RexExpression.Literal)) {
+ return -1;
+ }
+ int columnId = ((RexExpression.InputRef) inputRef).getIndex() - leftColumnSize;
+ return columnId >= 0 && columnId < numRightColumns ? columnId : -1;
+ }
+
+ /// Returns the literal value of a condition that [#getConstantEqualityColumnId] accepted.
+ @Nullable
+ private static Object getConstantValue(RexExpression condition) {
+ List<RexExpression> operands = ((RexExpression.FunctionCall) condition).getFunctionOperands();
+ RexExpression literal = operands.get(0) instanceof RexExpression.Literal ? operands.get(0) : operands.get(1);
+ return ((RexExpression.Literal) literal).getValue();
+ }
+
+ /// Converts a literal value to the representation that the dimension table stores.
+ ///
+ /// A literal already holds Pinot's internal value, but its numeric width follows the type that the planner gave the
+ /// literal, which can be narrower or wider than the dimension column. [PrimaryKey] compares values with `equals`,
+ /// where an `Integer` never equals a `Long`, so a literal of the wrong width silently misses every row.
+ ///
+ /// The switch rejects every type it cannot convert, rather than passing the value through. A value that does not
+ /// 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
+ /// 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.
Review Comment:
makes sense, thanks for the explanation!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19210:
URL: https://github.com/apache/pinot/pull/19210#issuecomment-5247312027
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19210?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`a2f96c4`](https://app.codecov.io/gh/apache/pinot/commit/a2f96c487ec403302648177e88466143d4331977?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`69034fa`](https://app.codecov.io/gh/apache/pinot/commit/69034fa083af554becf1c61af4ff7ffcaff30111?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (a2f96c4) and HEAD (69034fa). Click for more details.
>
> <details><summary>HEAD has 14 uploads less than BASE</summary>
>
>| Flag | BASE (a2f96c4) | HEAD (69034fa) |
>|------|------|------|
>|lane-a|2|1|
>|integration|4|2|
>|java-25|5|2|
>|integration1|2|0|
>|temurin|5|2|
>|lane-b|2|1|
>|unittests2|1|0|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19210 +/- ##
=============================================
- Coverage 38.95% 0.00% -38.96%
=============================================
Files 3452 3 -3449
Lines 218708 6 -218702
Branches 34789 0 -34789
=============================================
- Hits 85196 0 -85196
+ Misses 125746 6 -125740
+ Partials 7766 0 -7766
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-38.96%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-38.96%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19210/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19210?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19210:
URL: https://github.com/apache/pinot/pull/19210#discussion_r3761929623
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/LookupJoinOperatorTest.java:
##########
@@ -0,0 +1,237 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.query.runtime.operator;
+
+import java.math.BigDecimal;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
+
+
+/// Tests [LookupJoinOperator#compileKeyPlan], which decides where each value of the dimension table lookup key comes
+/// from.
+///
+/// The dimension table is a hash map keyed by the primary key values, so a key is only usable when it holds one value
+/// per primary key column, in the order the dimension table schema declares them, and with the stored type of each
+/// column. These tests cover the cases that a query alone cannot reach, such as a null literal and a literal of the
+/// wrong numeric width. End-to-end coverage is in `LookupJoin.json`.
+public class LookupJoinOperatorTest {
+ private static final String TABLE_NAME = "dim_tbl_OFFLINE";
+
+ /// Dimension table columns, in the order that the leaf stage reports them. The order is not the primary key order,
+ /// which is what makes the key positions worth testing.
+ private static final String[] RIGHT_COLUMNS = {"currency", "rate", "rate_start_date"};
+ private static final DataSchema RIGHT_SCHEMA = new DataSchema(RIGHT_COLUMNS,
+ new ColumnDataType[]{ColumnDataType.STRING, ColumnDataType.INT, ColumnDataType.LONG});
+ private static final List<String> PRIMARY_KEY_COLUMNS = List.of("currency", "rate_start_date");
+
+ /// The fact table has two columns, so the dimension table columns start at index 2 of the joined row.
+ private static final int LEFT_COLUMN_SIZE = 2;
+
+ @Test
+ public void testKeyPositionsFollowPrimaryKeyOrderNotConditionOrder() {
+ // ON dim.rate_start_date = fact.col1 AND dim.currency = fact.col0
+ // The conditions are in reverse primary key order, so the key values must still land in primary key order.
+ LookupJoinOperator.KeyPlan keyPlan =
+ compileKeyPlan(List.of(1, 0), List.of(2, 0), List.of());
Review Comment:
Good suggestion, done.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,194 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
_nonEquiEvaluators = new ArrayList<>(nonEquiConditions.size());
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition, _resultSchema));
}
+
+ KeyPlan keyPlan =
+ compileKeyPlan(node, rightTableName, _rightTable.getPrimaryKeyColumns(), _rightInput.getDataSchema(),
+ _leftColumnSize);
+ _keySize = keyPlan._sources.length;
+ _keySources = keyPlan._sources;
+ _keyConstants = keyPlan._constants;
+ _neverMatches = keyPlan._neverMatches;
+ }
+
+ /// Works out where each value of the lookup key comes from.
+ ///
+ /// The key has one position per dimension table primary key column, in the order the dimension table schema declares
+ /// them. Each position is bound in two passes:
+ ///
+ /// 1. Equi-join keys. `rightKeys[i]` names a dimension column, and that column's position in the primary key decides
+ /// where `leftKeys[i]` lands. This is what makes the key independent of the order of the join condition.
+ /// 2. Constants. A non-equi condition of the form `dim_column = literal` binds a position that pass 1 left open.
+ /// A constant never replaces an equi-join key, because the equi-join key is not kept anywhere else and dropping it
+ /// would silently widen the join. A constant that pass 1 already bound stays in [#_nonEquiEvaluators] and runs as
+ /// a filter after the lookup, which is what the SQL semantics require.
+ ///
+ /// The method rejects a join condition that cannot produce exactly one value per primary key column. Every rejected
+ /// case returned no rows or wrong rows before this validation existed, so an error is the better outcome. This is
+ /// also the contract that the single-stage `lookup` transform function enforces.
+ @VisibleForTesting
+ static KeyPlan compileKeyPlan(JoinNode node, String tableName, @Nullable List<String> primaryKeyColumns,
+ DataSchema rightSchema, int leftColumnSize) {
+ Preconditions.checkState(CollectionUtils.isNotEmpty(primaryKeyColumns),
+ "Failed to find primary key columns for dimension table: %s", tableName);
+ String[] rightColumns = rightSchema.getColumnNames();
+ int keySize = primaryKeyColumns.size();
+ int[] sources = new int[keySize];
+ Arrays.fill(sources, KEY_SOURCE_UNBOUND);
+ Object[] constants = new Object[keySize];
+
+ // Pass 1: bind key positions from the equi-join keys.
+ List<Integer> leftKeys = node.getLeftKeys();
+ List<Integer> rightKeys = node.getRightKeys();
+ int numEquiKeys = leftKeys.size();
+ for (int i = 0; i < numEquiKeys; i++) {
+ String rightColumn = rightColumns[rightKeys.get(i)];
+ int keyPosition = primaryKeyColumns.indexOf(rightColumn);
+ Preconditions.checkState(keyPosition >= 0,
+ "Lookup join on dimension table: %s has a join key on column: %s, which is not a primary key column. "
+ + "Primary key columns: %s", tableName, rightColumn, primaryKeyColumns);
+ Preconditions.checkState(sources[keyPosition] == KEY_SOURCE_UNBOUND,
+ "Lookup join on dimension table: %s has multiple join keys on primary key column: %s", tableName,
+ rightColumn);
+ sources[keyPosition] = leftKeys.get(i);
+ }
+
+ // Pass 2: bind the remaining key positions from constant equality conditions.
+ boolean neverMatches = false;
+ for (RexExpression nonEquiCondition : node.getNonEquiConditions()) {
+ int rightColumnId = getConstantEqualityColumnId(nonEquiCondition, leftColumnSize, rightColumns.length);
+ if (rightColumnId < 0) {
+ continue;
+ }
+ int keyPosition = primaryKeyColumns.indexOf(rightColumns[rightColumnId]);
+ if (keyPosition < 0 || sources[keyPosition] != KEY_SOURCE_UNBOUND) {
+ continue;
+ }
+ sources[keyPosition] = KEY_SOURCE_CONSTANT;
+ Object value = getConstantValue(nonEquiCondition);
+ constants[keyPosition] =
+ toStoredValue(value, rightSchema.getColumnDataType(rightColumnId), tableName, rightColumns[rightColumnId]);
+ neverMatches |= constants[keyPosition] == null;
+ }
+
+ List<String> unboundColumns = new ArrayList<>();
+ for (int i = 0; i < keySize; i++) {
+ if (sources[i] == KEY_SOURCE_UNBOUND) {
+ unboundColumns.add(primaryKeyColumns.get(i));
+ }
+ }
+ Preconditions.checkState(unboundColumns.isEmpty(),
+ "Lookup join on dimension table: %s cannot determine primary key columns: %s from the join condition. "
+ + "A lookup join reads the dimension table by primary key, so the join condition must have an equality on "
+ + "every primary key column: %s. Add the missing conditions, or remove the lookup join hint to use a hash "
+ + "join instead.", tableName, unboundColumns, primaryKeyColumns);
+ return new KeyPlan(sources, constants, neverMatches);
+ }
+
+ /// Returns the dimension table column id of a `dim_column = literal` condition, or -1 when the condition does not
+ /// have that shape. Only an equality against a single literal can serve as a key value. A condition such as
+ /// `dim_column IN ('a', 'b')` reaches this method as a disjunction and returns -1, because a hash lookup cannot read
+ /// a set of keys.
+ ///
+ /// Non-equi conditions index the joined row, so the dimension table columns start at `leftColumnSize`.
+ private static int getConstantEqualityColumnId(RexExpression condition, int leftColumnSize, int numRightColumns) {
+ if (!(condition instanceof RexExpression.FunctionCall)) {
+ return -1;
+ }
+ RexExpression.FunctionCall functionCall = (RexExpression.FunctionCall) condition;
+ if (!functionCall.getFunctionName().equals(SqlKind.EQUALS.name())) {
+ return -1;
+ }
+ List<RexExpression> operands = functionCall.getFunctionOperands();
+ if (operands.size() != 2) {
+ return -1;
+ }
+ RexExpression inputRef = operands.get(0) instanceof RexExpression.InputRef ? operands.get(0) : operands.get(1);
+ RexExpression literal = operands.get(0) instanceof RexExpression.InputRef ? operands.get(1) : operands.get(0);
+ if (!(inputRef instanceof RexExpression.InputRef) || !(literal instanceof RexExpression.Literal)) {
+ return -1;
+ }
+ int columnId = ((RexExpression.InputRef) inputRef).getIndex() - leftColumnSize;
+ return columnId >= 0 && columnId < numRightColumns ? columnId : -1;
+ }
+
+ /// Returns the literal value of a condition that [#getConstantEqualityColumnId] accepted.
+ @Nullable
+ private static Object getConstantValue(RexExpression condition) {
+ List<RexExpression> operands = ((RexExpression.FunctionCall) condition).getFunctionOperands();
+ RexExpression literal = operands.get(0) instanceof RexExpression.Literal ? operands.get(0) : operands.get(1);
+ return ((RexExpression.Literal) literal).getValue();
+ }
+
+ /// Converts a literal value to the representation that the dimension table stores.
+ ///
+ /// A literal already holds Pinot's internal value, but its numeric width follows the type that the planner gave the
+ /// literal, which can be narrower or wider than the dimension column. [PrimaryKey] compares values with `equals`,
+ /// where an `Integer` never equals a `Long`, so a literal of the wrong width silently misses every row.
+ ///
+ /// The switch rejects every type it cannot convert, rather than passing the value through. A value that does not
+ /// 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
+ /// 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.
Review Comment:
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.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,194 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
_nonEquiEvaluators = new ArrayList<>(nonEquiConditions.size());
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition, _resultSchema));
}
+
+ KeyPlan keyPlan =
+ compileKeyPlan(node, rightTableName, _rightTable.getPrimaryKeyColumns(), _rightInput.getDataSchema(),
+ _leftColumnSize);
+ _keySize = keyPlan._sources.length;
+ _keySources = keyPlan._sources;
+ _keyConstants = keyPlan._constants;
+ _neverMatches = keyPlan._neverMatches;
+ }
+
+ /// Works out where each value of the lookup key comes from.
+ ///
+ /// The key has one position per dimension table primary key column, in the order the dimension table schema declares
+ /// them. Each position is bound in two passes:
+ ///
+ /// 1. Equi-join keys. `rightKeys[i]` names a dimension column, and that column's position in the primary key decides
+ /// where `leftKeys[i]` lands. This is what makes the key independent of the order of the join condition.
+ /// 2. Constants. A non-equi condition of the form `dim_column = literal` binds a position that pass 1 left open.
+ /// A constant never replaces an equi-join key, because the equi-join key is not kept anywhere else and dropping it
+ /// would silently widen the join. A constant that pass 1 already bound stays in [#_nonEquiEvaluators] and runs as
+ /// a filter after the lookup, which is what the SQL semantics require.
+ ///
+ /// The method rejects a join condition that cannot produce exactly one value per primary key column. Every rejected
+ /// case returned no rows or wrong rows before this validation existed, so an error is the better outcome. This is
+ /// also the contract that the single-stage `lookup` transform function enforces.
+ @VisibleForTesting
+ static KeyPlan compileKeyPlan(JoinNode node, String tableName, @Nullable List<String> primaryKeyColumns,
+ DataSchema rightSchema, int leftColumnSize) {
+ Preconditions.checkState(CollectionUtils.isNotEmpty(primaryKeyColumns),
+ "Failed to find primary key columns for dimension table: %s", tableName);
+ String[] rightColumns = rightSchema.getColumnNames();
+ int keySize = primaryKeyColumns.size();
+ int[] sources = new int[keySize];
+ Arrays.fill(sources, KEY_SOURCE_UNBOUND);
+ Object[] constants = new Object[keySize];
+
+ // Pass 1: bind key positions from the equi-join keys.
+ List<Integer> leftKeys = node.getLeftKeys();
+ List<Integer> rightKeys = node.getRightKeys();
+ int numEquiKeys = leftKeys.size();
+ for (int i = 0; i < numEquiKeys; i++) {
+ String rightColumn = rightColumns[rightKeys.get(i)];
+ int keyPosition = primaryKeyColumns.indexOf(rightColumn);
+ Preconditions.checkState(keyPosition >= 0,
+ "Lookup join on dimension table: %s has a join key on column: %s, which is not a primary key column. "
+ + "Primary key columns: %s", tableName, rightColumn, primaryKeyColumns);
+ Preconditions.checkState(sources[keyPosition] == KEY_SOURCE_UNBOUND,
+ "Lookup join on dimension table: %s has multiple join keys on primary key column: %s", tableName,
+ rightColumn);
+ sources[keyPosition] = leftKeys.get(i);
+ }
+
+ // Pass 2: bind the remaining key positions from constant equality conditions.
+ boolean neverMatches = false;
+ for (RexExpression nonEquiCondition : node.getNonEquiConditions()) {
+ int rightColumnId = getConstantEqualityColumnId(nonEquiCondition, leftColumnSize, rightColumns.length);
+ if (rightColumnId < 0) {
+ continue;
+ }
+ int keyPosition = primaryKeyColumns.indexOf(rightColumns[rightColumnId]);
+ if (keyPosition < 0 || sources[keyPosition] != KEY_SOURCE_UNBOUND) {
+ continue;
+ }
+ sources[keyPosition] = KEY_SOURCE_CONSTANT;
+ Object value = getConstantValue(nonEquiCondition);
+ constants[keyPosition] =
+ toStoredValue(value, rightSchema.getColumnDataType(rightColumnId), tableName, rightColumns[rightColumnId]);
+ neverMatches |= constants[keyPosition] == null;
+ }
+
+ List<String> unboundColumns = new ArrayList<>();
+ for (int i = 0; i < keySize; i++) {
+ if (sources[i] == KEY_SOURCE_UNBOUND) {
+ unboundColumns.add(primaryKeyColumns.get(i));
+ }
+ }
+ Preconditions.checkState(unboundColumns.isEmpty(),
+ "Lookup join on dimension table: %s cannot determine primary key columns: %s from the join condition. "
+ + "A lookup join reads the dimension table by primary key, so the join condition must have an equality on "
+ + "every primary key column: %s. Add the missing conditions, or remove the lookup join hint to use a hash "
+ + "join instead.", tableName, unboundColumns, primaryKeyColumns);
+ return new KeyPlan(sources, constants, neverMatches);
+ }
+
+ /// Returns the dimension table column id of a `dim_column = literal` condition, or -1 when the condition does not
+ /// have that shape. Only an equality against a single literal can serve as a key value. A condition such as
+ /// `dim_column IN ('a', 'b')` reaches this method as a disjunction and returns -1, because a hash lookup cannot read
+ /// a set of keys.
+ ///
+ /// Non-equi conditions index the joined row, so the dimension table columns start at `leftColumnSize`.
+ private static int getConstantEqualityColumnId(RexExpression condition, int leftColumnSize, int numRightColumns) {
+ if (!(condition instanceof RexExpression.FunctionCall)) {
+ return -1;
+ }
+ RexExpression.FunctionCall functionCall = (RexExpression.FunctionCall) condition;
+ if (!functionCall.getFunctionName().equals(SqlKind.EQUALS.name())) {
+ return -1;
+ }
+ List<RexExpression> operands = functionCall.getFunctionOperands();
+ if (operands.size() != 2) {
+ return -1;
+ }
+ RexExpression inputRef = operands.get(0) instanceof RexExpression.InputRef ? operands.get(0) : operands.get(1);
+ RexExpression literal = operands.get(0) instanceof RexExpression.InputRef ? operands.get(1) : operands.get(0);
+ if (!(inputRef instanceof RexExpression.InputRef) || !(literal instanceof RexExpression.Literal)) {
+ return -1;
+ }
+ int columnId = ((RexExpression.InputRef) inputRef).getIndex() - leftColumnSize;
+ return columnId >= 0 && columnId < numRightColumns ? columnId : -1;
+ }
+
+ /// Returns the literal value of a condition that [#getConstantEqualityColumnId] accepted.
+ @Nullable
+ private static Object getConstantValue(RexExpression condition) {
+ List<RexExpression> operands = ((RexExpression.FunctionCall) condition).getFunctionOperands();
+ RexExpression literal = operands.get(0) instanceof RexExpression.Literal ? operands.get(0) : operands.get(1);
+ return ((RexExpression.Literal) literal).getValue();
+ }
+
+ /// Converts a literal value to the representation that the dimension table stores.
+ ///
+ /// A literal already holds Pinot's internal value, but its numeric width follows the type that the planner gave the
+ /// literal, which can be narrower or wider than the dimension column. [PrimaryKey] compares values with `equals`,
+ /// where an `Integer` never equals a `Long`, so a literal of the wrong width silently misses every row.
+ ///
+ /// The switch rejects every type it cannot convert, rather than passing the value through. A value that does not
+ /// 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
Review Comment:
I think the same hazard exists elsewhere too TBH, so this isn't lookup join specific. I've updated this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #19210:
URL: https://github.com/apache/pinot/pull/19210
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19210:
URL: https://github.com/apache/pinot/pull/19210#discussion_r3770909271
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,179 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
Review Comment:
Seems redundant to add a test for a clearly unsupported path.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "timothy-e (via GitHub)" <gi...@apache.org>.
timothy-e commented on code in PR #19210:
URL: https://github.com/apache/pinot/pull/19210#discussion_r3758955388
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,194 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
_nonEquiEvaluators = new ArrayList<>(nonEquiConditions.size());
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition, _resultSchema));
}
+
+ KeyPlan keyPlan =
+ compileKeyPlan(node, rightTableName, _rightTable.getPrimaryKeyColumns(), _rightInput.getDataSchema(),
+ _leftColumnSize);
+ _keySize = keyPlan._sources.length;
+ _keySources = keyPlan._sources;
+ _keyConstants = keyPlan._constants;
+ _neverMatches = keyPlan._neverMatches;
+ }
+
+ /// Works out where each value of the lookup key comes from.
+ ///
+ /// The key has one position per dimension table primary key column, in the order the dimension table schema declares
+ /// them. Each position is bound in two passes:
+ ///
+ /// 1. Equi-join keys. `rightKeys[i]` names a dimension column, and that column's position in the primary key decides
+ /// where `leftKeys[i]` lands. This is what makes the key independent of the order of the join condition.
+ /// 2. Constants. A non-equi condition of the form `dim_column = literal` binds a position that pass 1 left open.
+ /// A constant never replaces an equi-join key, because the equi-join key is not kept anywhere else and dropping it
+ /// would silently widen the join. A constant that pass 1 already bound stays in [#_nonEquiEvaluators] and runs as
+ /// a filter after the lookup, which is what the SQL semantics require.
+ ///
+ /// The method rejects a join condition that cannot produce exactly one value per primary key column. Every rejected
+ /// case returned no rows or wrong rows before this validation existed, so an error is the better outcome. This is
+ /// also the contract that the single-stage `lookup` transform function enforces.
+ @VisibleForTesting
+ static KeyPlan compileKeyPlan(JoinNode node, String tableName, @Nullable List<String> primaryKeyColumns,
+ DataSchema rightSchema, int leftColumnSize) {
+ Preconditions.checkState(CollectionUtils.isNotEmpty(primaryKeyColumns),
+ "Failed to find primary key columns for dimension table: %s", tableName);
+ String[] rightColumns = rightSchema.getColumnNames();
+ int keySize = primaryKeyColumns.size();
+ int[] sources = new int[keySize];
+ Arrays.fill(sources, KEY_SOURCE_UNBOUND);
+ Object[] constants = new Object[keySize];
+
+ // Pass 1: bind key positions from the equi-join keys.
+ List<Integer> leftKeys = node.getLeftKeys();
+ List<Integer> rightKeys = node.getRightKeys();
+ int numEquiKeys = leftKeys.size();
+ for (int i = 0; i < numEquiKeys; i++) {
+ String rightColumn = rightColumns[rightKeys.get(i)];
+ int keyPosition = primaryKeyColumns.indexOf(rightColumn);
+ Preconditions.checkState(keyPosition >= 0,
+ "Lookup join on dimension table: %s has a join key on column: %s, which is not a primary key column. "
+ + "Primary key columns: %s", tableName, rightColumn, primaryKeyColumns);
+ Preconditions.checkState(sources[keyPosition] == KEY_SOURCE_UNBOUND,
+ "Lookup join on dimension table: %s has multiple join keys on primary key column: %s", tableName,
+ rightColumn);
+ sources[keyPosition] = leftKeys.get(i);
+ }
+
+ // Pass 2: bind the remaining key positions from constant equality conditions.
+ boolean neverMatches = false;
+ for (RexExpression nonEquiCondition : node.getNonEquiConditions()) {
+ int rightColumnId = getConstantEqualityColumnId(nonEquiCondition, leftColumnSize, rightColumns.length);
+ if (rightColumnId < 0) {
+ continue;
+ }
+ int keyPosition = primaryKeyColumns.indexOf(rightColumns[rightColumnId]);
+ if (keyPosition < 0 || sources[keyPosition] != KEY_SOURCE_UNBOUND) {
+ continue;
+ }
+ sources[keyPosition] = KEY_SOURCE_CONSTANT;
+ Object value = getConstantValue(nonEquiCondition);
+ constants[keyPosition] =
+ toStoredValue(value, rightSchema.getColumnDataType(rightColumnId), tableName, rightColumns[rightColumnId]);
+ neverMatches |= constants[keyPosition] == null;
+ }
+
+ List<String> unboundColumns = new ArrayList<>();
+ for (int i = 0; i < keySize; i++) {
+ if (sources[i] == KEY_SOURCE_UNBOUND) {
+ unboundColumns.add(primaryKeyColumns.get(i));
+ }
+ }
+ Preconditions.checkState(unboundColumns.isEmpty(),
+ "Lookup join on dimension table: %s cannot determine primary key columns: %s from the join condition. "
+ + "A lookup join reads the dimension table by primary key, so the join condition must have an equality on "
+ + "every primary key column: %s. Add the missing conditions, or remove the lookup join hint to use a hash "
+ + "join instead.", tableName, unboundColumns, primaryKeyColumns);
+ return new KeyPlan(sources, constants, neverMatches);
+ }
+
+ /// Returns the dimension table column id of a `dim_column = literal` condition, or -1 when the condition does not
+ /// have that shape. Only an equality against a single literal can serve as a key value. A condition such as
+ /// `dim_column IN ('a', 'b')` reaches this method as a disjunction and returns -1, because a hash lookup cannot read
+ /// a set of keys.
+ ///
+ /// Non-equi conditions index the joined row, so the dimension table columns start at `leftColumnSize`.
+ private static int getConstantEqualityColumnId(RexExpression condition, int leftColumnSize, int numRightColumns) {
+ if (!(condition instanceof RexExpression.FunctionCall)) {
+ return -1;
+ }
+ RexExpression.FunctionCall functionCall = (RexExpression.FunctionCall) condition;
+ if (!functionCall.getFunctionName().equals(SqlKind.EQUALS.name())) {
+ return -1;
+ }
+ List<RexExpression> operands = functionCall.getFunctionOperands();
+ if (operands.size() != 2) {
+ return -1;
+ }
+ RexExpression inputRef = operands.get(0) instanceof RexExpression.InputRef ? operands.get(0) : operands.get(1);
+ RexExpression literal = operands.get(0) instanceof RexExpression.InputRef ? operands.get(1) : operands.get(0);
+ if (!(inputRef instanceof RexExpression.InputRef) || !(literal instanceof RexExpression.Literal)) {
+ return -1;
+ }
+ int columnId = ((RexExpression.InputRef) inputRef).getIndex() - leftColumnSize;
+ return columnId >= 0 && columnId < numRightColumns ? columnId : -1;
+ }
+
+ /// Returns the literal value of a condition that [#getConstantEqualityColumnId] accepted.
+ @Nullable
+ private static Object getConstantValue(RexExpression condition) {
+ List<RexExpression> operands = ((RexExpression.FunctionCall) condition).getFunctionOperands();
+ RexExpression literal = operands.get(0) instanceof RexExpression.Literal ? operands.get(0) : operands.get(1);
+ return ((RexExpression.Literal) literal).getValue();
+ }
+
+ /// Converts a literal value to the representation that the dimension table stores.
+ ///
+ /// A literal already holds Pinot's internal value, but its numeric width follows the type that the planner gave the
+ /// literal, which can be narrower or wider than the dimension column. [PrimaryKey] compares values with `equals`,
+ /// where an `Integer` never equals a `Long`, so a literal of the wrong width silently misses every row.
+ ///
+ /// The switch rejects every type it cannot convert, rather than passing the value through. A value that does not
+ /// 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
+ /// 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.
Review Comment:
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
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/LookupJoinOperatorTest.java:
##########
@@ -0,0 +1,237 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.query.runtime.operator;
+
+import java.math.BigDecimal;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.query.planner.logical.RexExpression;
+import org.apache.pinot.query.planner.plannode.JoinNode;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
+
+
+/// Tests [LookupJoinOperator#compileKeyPlan], which decides where each value of the dimension table lookup key comes
+/// from.
+///
+/// The dimension table is a hash map keyed by the primary key values, so a key is only usable when it holds one value
+/// per primary key column, in the order the dimension table schema declares them, and with the stored type of each
+/// column. These tests cover the cases that a query alone cannot reach, such as a null literal and a literal of the
+/// wrong numeric width. End-to-end coverage is in `LookupJoin.json`.
+public class LookupJoinOperatorTest {
+ private static final String TABLE_NAME = "dim_tbl_OFFLINE";
+
+ /// Dimension table columns, in the order that the leaf stage reports them. The order is not the primary key order,
+ /// which is what makes the key positions worth testing.
+ private static final String[] RIGHT_COLUMNS = {"currency", "rate", "rate_start_date"};
+ private static final DataSchema RIGHT_SCHEMA = new DataSchema(RIGHT_COLUMNS,
+ new ColumnDataType[]{ColumnDataType.STRING, ColumnDataType.INT, ColumnDataType.LONG});
+ private static final List<String> PRIMARY_KEY_COLUMNS = List.of("currency", "rate_start_date");
+
+ /// The fact table has two columns, so the dimension table columns start at index 2 of the joined row.
+ private static final int LEFT_COLUMN_SIZE = 2;
+
+ @Test
+ public void testKeyPositionsFollowPrimaryKeyOrderNotConditionOrder() {
+ // ON dim.rate_start_date = fact.col1 AND dim.currency = fact.col0
+ // The conditions are in reverse primary key order, so the key values must still land in primary key order.
+ LookupJoinOperator.KeyPlan keyPlan =
+ compileKeyPlan(List.of(1, 0), List.of(2, 0), List.of());
Review Comment:
If we added something like
```java
private static final int FACT_COL0 = 0;
private static final int FACT_COL1 = 1;
...
private static ... eq(int leftCol, int RightCol)
```
then we could make these tests look like
```java
compileKeyPlan(List.of(eq(FACT_COL1, DIM_RATE_START_DATE), eq(FACT_COL0, DIM_CURRENCY)), List.of())
```
instead of
```java
compileKeyPlan(List.of(1, 0), List.of(2, 0), List.of());
```
which makes the intention much easier to read
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,194 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
_nonEquiEvaluators = new ArrayList<>(nonEquiConditions.size());
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition, _resultSchema));
}
+
+ KeyPlan keyPlan =
+ compileKeyPlan(node, rightTableName, _rightTable.getPrimaryKeyColumns(), _rightInput.getDataSchema(),
+ _leftColumnSize);
+ _keySize = keyPlan._sources.length;
+ _keySources = keyPlan._sources;
+ _keyConstants = keyPlan._constants;
+ _neverMatches = keyPlan._neverMatches;
+ }
+
+ /// Works out where each value of the lookup key comes from.
+ ///
+ /// The key has one position per dimension table primary key column, in the order the dimension table schema declares
+ /// them. Each position is bound in two passes:
+ ///
+ /// 1. Equi-join keys. `rightKeys[i]` names a dimension column, and that column's position in the primary key decides
+ /// where `leftKeys[i]` lands. This is what makes the key independent of the order of the join condition.
+ /// 2. Constants. A non-equi condition of the form `dim_column = literal` binds a position that pass 1 left open.
+ /// A constant never replaces an equi-join key, because the equi-join key is not kept anywhere else and dropping it
+ /// would silently widen the join. A constant that pass 1 already bound stays in [#_nonEquiEvaluators] and runs as
+ /// a filter after the lookup, which is what the SQL semantics require.
+ ///
+ /// The method rejects a join condition that cannot produce exactly one value per primary key column. Every rejected
+ /// case returned no rows or wrong rows before this validation existed, so an error is the better outcome. This is
+ /// also the contract that the single-stage `lookup` transform function enforces.
+ @VisibleForTesting
+ static KeyPlan compileKeyPlan(JoinNode node, String tableName, @Nullable List<String> primaryKeyColumns,
+ DataSchema rightSchema, int leftColumnSize) {
+ Preconditions.checkState(CollectionUtils.isNotEmpty(primaryKeyColumns),
+ "Failed to find primary key columns for dimension table: %s", tableName);
+ String[] rightColumns = rightSchema.getColumnNames();
+ int keySize = primaryKeyColumns.size();
+ int[] sources = new int[keySize];
+ Arrays.fill(sources, KEY_SOURCE_UNBOUND);
+ Object[] constants = new Object[keySize];
+
+ // Pass 1: bind key positions from the equi-join keys.
+ List<Integer> leftKeys = node.getLeftKeys();
+ List<Integer> rightKeys = node.getRightKeys();
+ int numEquiKeys = leftKeys.size();
+ for (int i = 0; i < numEquiKeys; i++) {
+ String rightColumn = rightColumns[rightKeys.get(i)];
+ int keyPosition = primaryKeyColumns.indexOf(rightColumn);
+ Preconditions.checkState(keyPosition >= 0,
+ "Lookup join on dimension table: %s has a join key on column: %s, which is not a primary key column. "
+ + "Primary key columns: %s", tableName, rightColumn, primaryKeyColumns);
+ Preconditions.checkState(sources[keyPosition] == KEY_SOURCE_UNBOUND,
+ "Lookup join on dimension table: %s has multiple join keys on primary key column: %s", tableName,
+ rightColumn);
+ sources[keyPosition] = leftKeys.get(i);
+ }
+
+ // Pass 2: bind the remaining key positions from constant equality conditions.
+ boolean neverMatches = false;
+ for (RexExpression nonEquiCondition : node.getNonEquiConditions()) {
+ int rightColumnId = getConstantEqualityColumnId(nonEquiCondition, leftColumnSize, rightColumns.length);
+ if (rightColumnId < 0) {
+ continue;
+ }
+ int keyPosition = primaryKeyColumns.indexOf(rightColumns[rightColumnId]);
+ if (keyPosition < 0 || sources[keyPosition] != KEY_SOURCE_UNBOUND) {
+ continue;
+ }
+ sources[keyPosition] = KEY_SOURCE_CONSTANT;
+ Object value = getConstantValue(nonEquiCondition);
+ constants[keyPosition] =
+ toStoredValue(value, rightSchema.getColumnDataType(rightColumnId), tableName, rightColumns[rightColumnId]);
+ neverMatches |= constants[keyPosition] == null;
+ }
+
+ List<String> unboundColumns = new ArrayList<>();
+ for (int i = 0; i < keySize; i++) {
+ if (sources[i] == KEY_SOURCE_UNBOUND) {
+ unboundColumns.add(primaryKeyColumns.get(i));
+ }
+ }
+ Preconditions.checkState(unboundColumns.isEmpty(),
+ "Lookup join on dimension table: %s cannot determine primary key columns: %s from the join condition. "
+ + "A lookup join reads the dimension table by primary key, so the join condition must have an equality on "
+ + "every primary key column: %s. Add the missing conditions, or remove the lookup join hint to use a hash "
+ + "join instead.", tableName, unboundColumns, primaryKeyColumns);
+ return new KeyPlan(sources, constants, neverMatches);
+ }
+
+ /// Returns the dimension table column id of a `dim_column = literal` condition, or -1 when the condition does not
+ /// have that shape. Only an equality against a single literal can serve as a key value. A condition such as
+ /// `dim_column IN ('a', 'b')` reaches this method as a disjunction and returns -1, because a hash lookup cannot read
+ /// a set of keys.
+ ///
+ /// Non-equi conditions index the joined row, so the dimension table columns start at `leftColumnSize`.
+ private static int getConstantEqualityColumnId(RexExpression condition, int leftColumnSize, int numRightColumns) {
+ if (!(condition instanceof RexExpression.FunctionCall)) {
+ return -1;
+ }
+ RexExpression.FunctionCall functionCall = (RexExpression.FunctionCall) condition;
+ if (!functionCall.getFunctionName().equals(SqlKind.EQUALS.name())) {
+ return -1;
+ }
+ List<RexExpression> operands = functionCall.getFunctionOperands();
+ if (operands.size() != 2) {
+ return -1;
+ }
+ RexExpression inputRef = operands.get(0) instanceof RexExpression.InputRef ? operands.get(0) : operands.get(1);
+ RexExpression literal = operands.get(0) instanceof RexExpression.InputRef ? operands.get(1) : operands.get(0);
+ if (!(inputRef instanceof RexExpression.InputRef) || !(literal instanceof RexExpression.Literal)) {
+ return -1;
+ }
+ int columnId = ((RexExpression.InputRef) inputRef).getIndex() - leftColumnSize;
+ return columnId >= 0 && columnId < numRightColumns ? columnId : -1;
+ }
+
+ /// Returns the literal value of a condition that [#getConstantEqualityColumnId] accepted.
+ @Nullable
+ private static Object getConstantValue(RexExpression condition) {
+ List<RexExpression> operands = ((RexExpression.FunctionCall) condition).getFunctionOperands();
+ RexExpression literal = operands.get(0) instanceof RexExpression.Literal ? operands.get(0) : operands.get(1);
+ return ((RexExpression.Literal) literal).getValue();
+ }
+
+ /// Converts a literal value to the representation that the dimension table stores.
+ ///
+ /// A literal already holds Pinot's internal value, but its numeric width follows the type that the planner gave the
+ /// literal, which can be narrower or wider than the dimension column. [PrimaryKey] compares values with `equals`,
+ /// where an `Integer` never equals a `Long`, so a literal of the wrong width silently misses every row.
+ ///
+ /// The switch rejects every type it cannot convert, rather than passing the value through. A value that does not
+ /// 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
Review Comment:
How does BIG_DECIMAL work on other join types?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19210:
URL: https://github.com/apache/pinot/pull/19210#discussion_r3770889117
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -301,13 +311,39 @@ private void registerMockDimensionTable(String offlineTableName, Schema schema,
}
Object[] values = new Object[columns.length];
for (int i = 0; i < columns.length; i++) {
- values[i] = row.getValue(columns[i]);
+ values[i] = toStoredValue(row.getValue(columns[i]), schema, columns[i]);
}
return values;
});
DimensionTableDataManager.registerDimensionTable(offlineTableName, mockDimManager);
}
+ /// Converts a raw JSON value to the stored representation of its column, the same way a segment stores it.
+ @Nullable
+ private static Object toStoredValue(@Nullable Object value, Schema schema, String column) {
+ if (value == null) {
+ return null;
+ }
+ FieldSpec fieldSpec = schema.getFieldSpecFor(column);
+ if (fieldSpec == null) {
+ return value;
+ }
+ 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;
+ }
Review Comment:
This conversion is incomplete for supported scalar types. A BOOLEAN has stored type INT but arrives from JSON as a Boolean, so the Number cast throws; TIMESTAMP may need parsing, while BIG_DECIMAL and BYTES currently fall through with representations that differ from a real segment. That makes the mock diverge from `PinotSegmentRecordReader` and can produce false failures or misses. Reuse the existing `FieldSpec.DataType` conversion instead of maintaining a partial stored-type switch.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,179 @@ public LookupJoinOperator(OpChainExecutionContext context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // 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);
Review Comment:
The new SEMI/ANTI rejection path is not covered by the added tests, although it is a user-visible behavior change intended to replace an internal index error with this diagnostic. Add focused SEMI and ANTI lookup-join cases with a non-equi condition and assert this message, so both join-type branches remain protected.
##########
pinot-query-runtime/src/test/resources/queries/LookupJoin.json:
##########
@@ -75,5 +75,172 @@
"ignoreLiteMode": true
}
]
+ },
+ "lookup_join_literal_key": {
+ "comment": "Regression test for issue 19188. One dimension primary-key component comes from a literal in the join condition. Calcite classifies that condition as a non-equi condition, so it is absent from the join keys. Before the fix the lookup key held one value against a two-column primary key and the join returned 0 rows.",
+ "tables": {
+ "fact_tbl": {
+ "schema": [
+ {"name": "rate_start_date", "type": "LONG"}
+ ],
+ "inputs": [
+ [1]
+ ]
+ },
+ "dim_tbl": {
+ "schema": [
+ {"name": "currency", "type": "STRING"},
+ {"name": "rate_start_date", "type": "LONG"},
+ {"name": "rate", "type": "INT"}
+ ],
+ "inputs": [
+ ["gbp", 1, 125],
+ ["usd", 1, 100]
+ ],
+ "replicated": true,
+ "isDimTable": true,
+ "primaryKeyColumns": ["currency", "rate_start_date"]
+ }
+ },
+ "queries": [
+ {
+ "description": "Lookup join with a literal dimension primary-key component",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {dim_tbl}.currency, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON {dim_tbl}.currency = 'gbp' AND {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date",
+ "outputs": [
+ ["gbp", 125]
+ ],
+ "ignoreLiteMode": true
+ }
+ ]
+ },
+ "lookup_join_composite_key": {
+ "comment": "Covers lookup joins against a two-column dimension primary key. The lookup key must hold one value per primary key column, in the order the dimension table schema declares them. These cases check that the key is complete, that it is ordered by the primary key instead of by the join condition, and that a join condition which cannot produce a complete key gives an error instead of 0 rows.",
+ "tables": {
+ "fact_tbl": {
+ "schema": [
+ {"name": "currency", "type": "STRING"},
+ {"name": "rate_start_date", "type": "LONG"},
+ {"name": "amount", "type": "INT"}
+ ],
+ "inputs": [
+ ["gbp", 1, 10],
+ ["usd", 1, 20],
+ ["gbp", 2, 30],
+ ["eur", 1, 40],
+ ["eur", 9, 50]
+ ]
+ },
+ "dim_tbl": {
+ "schema": [
+ {"name": "currency", "type": "STRING"},
+ {"name": "rate_start_date", "type": "LONG"},
+ {"name": "rate", "type": "INT"}
+ ],
+ "inputs": [
+ ["gbp", 1, 125],
+ ["usd", 1, 100],
+ ["gbp", 2, 130]
+ ],
+ "replicated": true,
+ "isDimTable": true,
+ "primaryKeyColumns": ["currency", "rate_start_date"]
+ }
+ },
+ "queries": [
+ {
+ "description": "Literal primary-key component in the ON clause",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {fact_tbl}.amount, {dim_tbl}.currency, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON {dim_tbl}.currency = 'gbp' AND {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date",
+ "outputs": [
+ [10, "gbp", 125],
+ [20, "gbp", 125],
+ [30, "gbp", 130],
+ [40, "gbp", 125]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Same literal primary-key component, written in the WHERE clause. The planner moves it into the join condition, so the operator sees the same plan.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {fact_tbl}.amount, {dim_tbl}.currency, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date WHERE {dim_tbl}.currency = 'gbp'",
+ "outputs": [
+ [10, "gbp", 125],
+ [20, "gbp", 125],
+ [30, "gbp", 130],
+ [40, "gbp", 125]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Left join with a literal primary-key component. Before the fix every row was null-padded, which gave wrong values instead of missing rows.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} LEFT JOIN {dim_tbl} ON {dim_tbl}.currency = 'gbp' AND {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date",
+ "outputs": [
+ [10, 125],
+ [20, 125],
+ [30, 130],
+ [40, 125],
+ [50, null]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Both primary-key columns equi-joined, conditions written in primary key order",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON {dim_tbl}.currency = {fact_tbl}.currency AND {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date",
+ "outputs": [
+ [10, 125],
+ [20, 100],
+ [30, 130]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Both primary-key columns equi-joined, conditions written in reverse primary key order. Before the fix the key values were swapped and the join returned 0 rows.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date AND {dim_tbl}.currency = {fact_tbl}.currency",
+ "outputs": [
+ [10, 125],
+ [20, 100],
+ [30, 130]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "A literal on a primary-key column that an equi-join key already binds. The equi-join key builds the key and the literal runs as a filter after the lookup. If the literal replaced the equi-join key, the usd fact row would read the gbp dimension row and add a wrong row.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */ {fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON {dim_tbl}.currency = {fact_tbl}.currency AND {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date AND {dim_tbl}.currency = 'gbp'",
+ "outputs": [
+ [10, 125],
+ [30, 130]
+ ],
+ "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.",
Review Comment:
This description contradicts the implementation and PR contract: the operator deliberately does not convert constants; it only verifies the planner-provided stored type. Please describe that validation so this regression test does not document behavior that does not exist.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Build the lookup join key from the dimension table primary key [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19210:
URL: https://github.com/apache/pinot/pull/19210#issuecomment-5323999642
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/992
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org