zabetak commented on code in PR #4478:
URL: https://github.com/apache/hive/pull/4478#discussion_r1266714031
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -2631,6 +2631,21 @@ private RelNode genJoinRelNode(RelNode leftRel, String
leftTableAlias, RelNode r
unparseTranslator.addIdentifierTranslation((ASTNode) child);
}
namedColumns.add(columnName);
+
+ // if leftTableAlias is null, set it to the last tableAlias that
contains the
+ // column columnName
+ if (leftTableAlias == null) {
+ Map<String, Map<String, ColumnInfo>> leftRslvMap =
leftRR.getRslvMap();
+ for (String tableAlias: leftRR.getTableNames()) {
+ if (!leftRslvMap.containsKey(tableAlias)) {
+ continue;
+ }
Review Comment:
Maybe we can avoid the access to `getTableNames` and the if check if we
iterate over `leftRR.getRslvMap().entrySet()`.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -2631,6 +2631,21 @@ private RelNode genJoinRelNode(RelNode leftRel, String
leftTableAlias, RelNode r
unparseTranslator.addIdentifierTranslation((ASTNode) child);
}
namedColumns.add(columnName);
+
+ // if leftTableAlias is null, set it to the last tableAlias that
contains the
+ // column columnName
+ if (leftTableAlias == null) {
Review Comment:
Can we end up in a similar situation where the `rightTableAlias` is null and
generates an NPE?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -2631,6 +2631,21 @@ private RelNode genJoinRelNode(RelNode leftRel, String
leftTableAlias, RelNode r
unparseTranslator.addIdentifierTranslation((ASTNode) child);
}
namedColumns.add(columnName);
+
+ // if leftTableAlias is null, set it to the last tableAlias that
contains the
Review Comment:
Let's extract the new code into a small method with an appropriate name and
if necessary add javadoc to clarify what it does.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -2631,6 +2631,21 @@ private RelNode genJoinRelNode(RelNode leftRel, String
leftTableAlias, RelNode r
unparseTranslator.addIdentifierTranslation((ASTNode) child);
}
namedColumns.add(columnName);
+
+ // if leftTableAlias is null, set it to the last tableAlias that
contains the
+ // column columnName
+ if (leftTableAlias == null) {
+ Map<String, Map<String, ColumnInfo>> leftRslvMap =
leftRR.getRslvMap();
+ for (String tableAlias: leftRR.getTableNames()) {
+ if (!leftRslvMap.containsKey(tableAlias)) {
+ continue;
+ }
+ if (leftRslvMap.get(tableAlias).containsKey(columnName)) {
+ leftTableAlias = tableAlias;
Review Comment:
What happens if we find the alias in some table other than the last (right
most) one? Is it a valid query ? Do we have test cases?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -2631,6 +2631,21 @@ private RelNode genJoinRelNode(RelNode leftRel, String
leftTableAlias, RelNode r
unparseTranslator.addIdentifierTranslation((ASTNode) child);
}
namedColumns.add(columnName);
+
+ // if leftTableAlias is null, set it to the last tableAlias that
contains the
+ // column columnName
+ if (leftTableAlias == null) {
+ Map<String, Map<String, ColumnInfo>> leftRslvMap =
leftRR.getRslvMap();
+ for (String tableAlias: leftRR.getTableNames()) {
+ if (!leftRslvMap.containsKey(tableAlias)) {
+ continue;
+ }
+ if (leftRslvMap.get(tableAlias).containsKey(columnName)) {
+ leftTableAlias = tableAlias;
Review Comment:
What happens if we don't find the alias at all in any table? Should we
throw? Do we have test cases?
##########
ql/src/test/queries/clientpositive/join_using_clause.q:
##########
@@ -0,0 +1,69 @@
+
+create table test (
+ a int
+);
+
+insert into test values (1);
+
+create table test1 (
+ a int,
+ b int
+);
+
+insert into test1 values (1, 2);
+
+create table test2 (
+ a int,
+ b int,
+ c int
+);
+
+insert into test2 values (1, 2, 3);
+
+-- self join with 1 column
+select * from test t1
+join test t2 using(a)
+join test t3 using(a);
+
+explain extended select * from test t1
Review Comment:
`EXPLAIN EXTENDED` is quite verbose. Do we really need it to prove that the
NPE is fixed? If we need to verify that plan is correct isn't `EXPLAIN CBO`
sufficient?
--
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]
