[GitHub] [flink] fsk119 commented on a diff in pull request #18920: [FLINK-26361][hive] Add variables when create LogicFilter to fix "unexpected correlate variable $cor0" exception with Hive dialect

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

fsk119 commented on code in PR #18920:
URL: https://github.com/apache/flink/pull/18920#discussion_r905990672


##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/copy/HiveParserBaseSemanticAnalyzer.java:
##########
@@ -1862,6 +1867,65 @@ public static RelNode genValues(
                 rows);
     }
 
+    // traverse the given node to find all correlated variables

Review Comment:
   nit: It's better to use 
   ```
   /***/ 
   ```
   
   at the beginning of the method.



##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/copy/HiveParserBaseSemanticAnalyzer.java:
##########
@@ -1862,6 +1867,65 @@ public static RelNode genValues(
                 rows);
     }
 
+    // traverse the given node to find all correlated variables
+    public static Set<CorrelationId> getVariablesSet(RexNode rexNode) {

Review Comment:
   I think we'd better to keep the same name with the Hive. What about renaming to `traverseFilter` and adding the java doc to tell where the code comes from?



##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/copy/HiveParserBaseSemanticAnalyzer.java:
##########
@@ -1862,6 +1867,65 @@ public static RelNode genValues(
                 rows);
     }
 
+    // traverse the given node to find all correlated variables
+    public static Set<CorrelationId> getVariablesSet(RexNode rexNode) {
+        Set<CorrelationId> correlationVariables = new HashSet<>();
+        if (rexNode instanceof RexSubQuery) {
+            RexSubQuery rexSubQuery = (RexSubQuery) rexNode;
+            // we expect correlated variables in Filter only for now.
+            // also check case where operator has o inputs .e.g TableScan

Review Comment:
   o -> 0



##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/HiveParserUtils.java:
##########
@@ -339,6 +358,20 @@ public static RelNode genValuesRelNode(
         }
     }
 
+    // creates LogicFilter node
+    public static RelNode genFilterRelNode(
+            RelNode relNode, RexNode rexNode, Collection<CorrelationId> variables) {
+        Class[] argTypes = new Class[] {RelNode.class, RexNode.class, null};
+        argTypes[2] = useShadedImmutableSet ? shadedImmutableSetClz : immutableSetClz;

Review Comment:
   nit:
   
   ```
           Class[] argTypes = new Class[] {RelNode.class, RexNode.class, useShadedImmutableSet ? shadedImmutableSetClz : immutableSetClz};
   ```
   
   
   



-- 
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: issues-unsubscribe@flink.apache.org

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