siddharthteotia commented on code in PR #10565:
URL: https://github.com/apache/pinot/pull/10565#discussion_r1162863468
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -669,6 +682,26 @@ private void extractColumns(QueryContext query) {
}
}
+ // For JOIN queries, keep only the columns from the left table, and
remove the table name prefix
+ JoinContext join = query.getDataSource().getJoin();
+ if (join != null) {
+ Set<String> leftTableColumns = new HashSet<>();
+ String columnPrefix = join.getRawLeftTableName() + ".";
+ int prefixLength = columnPrefix.length();
+ for (String column : columns) {
+ if (column.startsWith(columnPrefix)) {
+ leftTableColumns.add(column.substring(prefixLength));
+ }
+ }
+ join.getLeftJoinKey().getColumns(leftTableColumns);
+ columns = leftTableColumns;
Review Comment:
This seems incorrect. What if the select expression list columns also
contains columns from right table ?
So if the query is
```
SELECT T1.C1, T1.C2, T2.C3, T2.C4
FROM
T1 JOIN T2
ON T1.keyCol1 = T2.keyCol2
```
When we enter this block at linen 687, columns will be T1.C1, T1.C2, T2.C3,
T2.C4
columnPrefix = T1.
When we enter the for loop, prefix matches for first 2 expressions and we
pick up C1 and C2 and add to leftTableColumns
We then add `keyCol1` to `leftTableColumns` but I don't think
`expressionContext.getColumns` is going to remove the prefix and just return
the identifier
In any case, leftTableColumns seem to be `C1, C2 and keyCol1` which is what
we assign to `columns` at line 697. C3 and C4 seem to be ignored ?
--
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]
