Copilot commented on code in PR #19197:
URL: https://github.com/apache/pinot/pull/19197#discussion_r3752505037
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -64,6 +65,12 @@ public class LookupJoinOperator extends MultiStageOperator {
private final LeafOperator _rightInput;
private final JoinRelType _joinType;
private final int[] _leftKeyIds;
Review Comment:
After switching key construction to `_keyLeftIndices`/`_keyLiteralValues`,
`_leftKeyIds` is no longer referenced anywhere outside the constructor.
Consider removing `_leftKeyIds` and its constructor initialization block to
avoid carrying unused state.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -64,6 +65,12 @@ public class LookupJoinOperator extends MultiStageOperator {
private final LeafOperator _rightInput;
private final JoinRelType _joinType;
private final int[] _leftKeyIds;
+ private final int[] _rightKeyIds;
Review Comment:
`_rightKeyIds` is introduced here but never used later (and `rightKeys` is
already available as a `List<Integer>`). Keeping this extra field adds dead
code and can cause static-analysis noise.
This issue also appears on line 106 of the same file.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -96,11 +103,72 @@ public LookupJoinOperator(OpChainExecutionContext context,
MultiStageOperator le
_rightColumns = _rightInput.getDataSchema().getColumnNames();
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
+ List<Integer> rightKeys = node.getRightKeys();
+ _rightKeyIds = new int[rightKeys.size()];
+ for (int i = 0; i < rightKeys.size(); i++) {
+ _rightKeyIds[i] = rightKeys.get(i);
+ }
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
_nonEquiEvaluators = new ArrayList<>(nonEquiConditions.size());
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition,
_resultSchema));
}
+
+ // Build a complete lookup key in the dimension table's primary key column
order. When a join
+ // condition supplies a dimension primary key component as a literal (e.g.
"dim_tbl.currency = 'gbp'"),
+ // Calcite's analyzeCondition() classifies it as a non-equi condition
rather than an equi-join key, so it
+ // is absent from leftKeys/rightKeys. The lookup key must still include
that component, otherwise the
+ // lookup misses and returns 0 rows. Fill each primary key position from
either the corresponding left
+ // column (equi-join) or the literal value (non-equi condition).
+ List<String> primaryKeyColumns = _rightTable.getPrimaryKeyColumns();
+ Preconditions.checkState(primaryKeyColumns != null &&
!primaryKeyColumns.isEmpty(),
+ "Dimension table must have primary key columns for lookup join");
+ _keyLeftIndices = new int[primaryKeyColumns.size()];
Review Comment:
This now hard-depends on `DimensionTableDataManager#getPrimaryKeyColumns()`
being implemented; otherwise it throws during operator construction. The
lookup-join unit tests register a Mockito `DimensionTableDataManager` mock (see
`ResourceBasedQueriesTest#registerMockDimensionTable`) that stubs
`containsKey()` / `lookupValues()` but not `getPrimaryKeyColumns()`, so it will
return null and fail all lookup-join test cases unless the mock is updated to
`when(mockDimManager.getPrimaryKeyColumns()).thenReturn(primaryKeyColumns)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
