timothy-e commented on code in PR #19200:
URL: https://github.com/apache/pinot/pull/19200#discussion_r3751106580
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -101,6 +110,75 @@ public LookupJoinOperator(OpChainExecutionContext context,
MultiStageOperator le
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition,
_resultSchema));
}
+
+ // Build the key assembly plan for the dimension table's primary key.
+ // When a join condition includes a literal (e.g. dim.currency = 'gbp'),
the literal value must be used as a
+ // key component rather than treated as a non-equi post-filter only.
Without this, the lookup key would be
+ // incomplete (missing the literal-matched PK column) and the lookup would
return null.
+ List<String> rightPkColumns = _rightTable.getPrimaryKeyColumns();
+ _keyColumnCount = rightPkColumns.size();
+ _keyColumnLeftIds = new int[_keyColumnCount];
+ _keyColumnLiteralValues = new Object[_keyColumnCount];
+
+ // Initialize all PK columns as unmatched (placeholder -1, null literal)
+ for (int i = 0; i < _keyColumnCount; i++) {
+ _keyColumnLeftIds[i] = -1;
+ _keyColumnLiteralValues[i] = null;
+ }
Review Comment:
This uses the same representation for “unassigned PK component” and “literal
NULL”, and makes missing PK slots look intentionally mapped to NULL, which
changes the behaviour more than what we probably intend to do.
(e.g. right now, what happens when a lookup join doesn't have the full PK
specified? we should be careful about changing that)
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -101,6 +110,75 @@ public LookupJoinOperator(OpChainExecutionContext context,
MultiStageOperator le
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition,
_resultSchema));
}
+
+ // Build the key assembly plan for the dimension table's primary key.
+ // When a join condition includes a literal (e.g. dim.currency = 'gbp'),
the literal value must be used as a
+ // key component rather than treated as a non-equi post-filter only.
Without this, the lookup key would be
+ // incomplete (missing the literal-matched PK column) and the lookup would
return null.
+ List<String> rightPkColumns = _rightTable.getPrimaryKeyColumns();
+ _keyColumnCount = rightPkColumns.size();
+ _keyColumnLeftIds = new int[_keyColumnCount];
+ _keyColumnLiteralValues = new Object[_keyColumnCount];
+
+ // Initialize all PK columns as unmatched (placeholder -1, null literal)
+ for (int i = 0; i < _keyColumnCount; i++) {
+ _keyColumnLeftIds[i] = -1;
+ _keyColumnLiteralValues[i] = null;
+ }
+
+ // Build a map from right column name to its index in the right schema
+ Map<String, Integer> rightColIndex = new HashMap<>();
+ for (int i = 0; i < _rightColumns.length; i++) {
+ rightColIndex.put(_rightColumns[i], i);
+ }
Review Comment:
this map is unused
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -101,6 +110,75 @@ public LookupJoinOperator(OpChainExecutionContext context,
MultiStageOperator le
for (RexExpression nonEquiCondition : nonEquiConditions) {
_nonEquiEvaluators.add(TransformOperandFactory.getTransformOperand(nonEquiCondition,
_resultSchema));
}
+
+ // Build the key assembly plan for the dimension table's primary key.
+ // When a join condition includes a literal (e.g. dim.currency = 'gbp'),
the literal value must be used as a
+ // key component rather than treated as a non-equi post-filter only.
Without this, the lookup key would be
+ // incomplete (missing the literal-matched PK column) and the lookup would
return null.
+ List<String> rightPkColumns = _rightTable.getPrimaryKeyColumns();
+ _keyColumnCount = rightPkColumns.size();
+ _keyColumnLeftIds = new int[_keyColumnCount];
+ _keyColumnLiteralValues = new Object[_keyColumnCount];
+
+ // Initialize all PK columns as unmatched (placeholder -1, null literal)
+ for (int i = 0; i < _keyColumnCount; i++) {
+ _keyColumnLeftIds[i] = -1;
+ _keyColumnLiteralValues[i] = null;
+ }
+
+ // Build a map from right column name to its index in the right schema
+ Map<String, Integer> rightColIndex = new HashMap<>();
+ for (int i = 0; i < _rightColumns.length; i++) {
+ rightColIndex.put(_rightColumns[i], i);
+ }
+
+ // Map the equi-join left keys to PK columns via the right-side column
indices
+ List<Integer> rightKeys = node.getRightKeys();
+ for (int i = 0; i < _leftKeyIds.length && i < rightKeys.size(); i++) {
+ int rightKeyIdx = rightKeys.get(i);
+ if (rightKeyIdx >= 0 && rightKeyIdx < _rightColumns.length) {
+ String rightColName = _rightColumns[rightKeyIdx];
+ int pkPos = rightPkColumns.indexOf(rightColName);
+ if (pkPos >= 0) {
+ _keyColumnLeftIds[pkPos] = _leftKeyIds[i];
+ // _keyColumnLiteralValues[pkPos] stays null (left-key mode)
+ }
+ }
+ }
+
+ // Map literal-based equality conditions (e.g. dim.currency = 'gbp') to PK
columns
+ for (RexExpression nonEquiCondition : nonEquiConditions) {
+ if (nonEquiCondition instanceof RexExpression.FunctionCall) {
+ RexExpression.FunctionCall func = (RexExpression.FunctionCall)
nonEquiCondition;
+ if (("EQUALS".equals(func.getFunctionName()) ||
"=".equals(func.getFunctionName()))
+ && func.getFunctionOperands().size() == 2) {
+ RexExpression op1 = func.getFunctionOperands().get(0);
+ RexExpression op2 = func.getFunctionOperands().get(1);
+ RexExpression.InputRef inputRef = null;
+ RexExpression.Literal literal = null;
+ if (op1 instanceof RexExpression.InputRef && op2 instanceof
RexExpression.Literal) {
+ inputRef = (RexExpression.InputRef) op1;
+ literal = (RexExpression.Literal) op2;
+ } else if (op2 instanceof RexExpression.InputRef && op1 instanceof
RexExpression.Literal) {
+ inputRef = (RexExpression.InputRef) op2;
+ literal = (RexExpression.Literal) op1;
+ }
+ if (inputRef != null && literal != null) {
+ // The InputRef index is in the result schema (left + right
columns concatenated).
+ // Subtract _leftColumnSize to get the index into the right schema.
+ int rightIdx = inputRef.getIndex() - _leftColumnSize;
+ if (rightIdx >= 0 && rightIdx < _rightColumns.length) {
+ String rightColName = _rightColumns[rightIdx];
+ int pkPos = rightPkColumns.indexOf(rightColName);
+ if (pkPos >= 0) {
+ _keyColumnLeftIds[pkPos] = -1; // literal mode
Review Comment:
This could cause a literal to overwrite a equi-join key, e.g. in a scenario
like this:
```sql
ON fact.currency = dim.currency
AND fact.rate_start_date = dim.rate_start_date
AND dim.currency = 'gbp'
```
The join would only validate `dim.currency=gbp`, not
`fact.currency=dim.currency`. Can you also add a test for this case?
--
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]
