[GitHub] [hive] jcamachor commented on a change in pull request #2302: [HIVE-25183] Inner Join condition parsing error in subquery

[GitBox <gi...@apache.org>]

jcamachor commented on a change in pull request #2302:
URL: https://github.com/apache/hive/pull/2302#discussion_r643653073



##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/type/JoinCondTypeCheckProcFactory.java
##########
@@ -104,12 +105,20 @@ private boolean hasTableAlias(JoinTypeCheckCtx ctx, String tabName, ASTNode expr
           tblAliasCnt++;
       }
 
+      if (tblAliasCnt == 0 && ctx.getOuterRR() != null) {

Review comment:
       Why do we do this check? Maybe add a comment to the code.

##########
File path: ql/src/test/queries/clientpositive/subquery_corr_join.q
##########
@@ -0,0 +1,69 @@
+create table alltypestiny(
+id int,
+int_col int,
+bigint_col bigint,
+bool_col boolean
+);
+
+insert into alltypestiny(id, int_col, bigint_col, bool_col) values
+(1, 1, 10, true),
+(2, 4, 5, false),
+(3, 5, 15, true),
+(10, 10, 30, false);
+
+create table alltypesagg(
+id int,
+int_col int,
+bool_col boolean
+);
+
+insert into alltypesagg(id, int_col, bool_col) values
+(1, 1, true),
+(2, 4, false),
+(5, 6, true),
+(null, null, false);
+
+select *
+from alltypesagg t1
+where t1.id not in
+    (select tt1.id
+     from alltypestiny tt1 inner JOIN alltypesagg tt2

Review comment:
       Can we add a q file with a negative test for outer joins? That will be useful to make sure that the query will fail for the time being, as expected.

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveFilterJoinRule.java
##########
@@ -90,6 +90,36 @@ public boolean matches(RelOptRuleCall call) {
 
   }
 
+  /**
+   * Rule that tries to push join conditions into its inputs
+   */
+  public static class HiveJoinConditionPushRule extends HiveFilterJoinRule {

Review comment:
       Isn't this the same as `HiveFilterJoinTransposeRule`? It should not be necessary.

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelDecorrelator.java
##########
@@ -34,6 +34,7 @@
 
 import javax.annotation.Nonnull;
 
+import org.apache.calcite.adapter.enumerable.EnumerableConvention;

Review comment:
       This does not seem needed?

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/type/JoinCondTypeCheckProcFactory.java
##########
@@ -194,6 +207,19 @@ private ColumnInfo getColInfo(JoinTypeCheckCtx ctx, String tabName, String colAl
         }
       }
 
+      if (cInfoToRet == null && ctx.getOuterRR() != null) {
+        for (RowResolver rr : ImmutableList.of(ctx.getOuterRR())) {

Review comment:
       Is the `ImmutableList.of` wrapping needed?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org