Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/23209 )
Change subject: IMPALA-13767: Do not treat CTEs as names of actual tables ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java: http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@191 PS3, Line 191: new NoopAuthorizationFactory(), null); > 1) Instead of type SqlNode, we can use type SqlWith, since it will only > contain "SqlWith" items I agree with you. > 2) I think we can combine the two structures currentlyVisitedSqlNodes and > withItemsMap into a LinkedHashMap variable? If we do that, we also wouldn't > need a "computeIfAbsent". When we find a new SqlWith node, we can put an > entry into the LinkedHashMap with an empty Set<TableName> as the value. In > that way, we no longer would have to "computeIfAbsent" because it would > always be there. Don't know if I missed something. If we combine the two structures currentlyVisitedSqlNodes and withItemsMap into a LinkedHashMap variable, then is it possible for us to retrieve the closest SqlWith ('closestSqlWith' at https://gerrit.cloudera.org/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java#217) in one step assuming that we only added SqlWith nodes to this LinkedHashMap? Or we will have to iterate over the LinkedHashMap? It looks like LinkedHashMap does not have a method like descendingIterator() that would enable us to traverse a list in reverse order. http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@202 PS3, Line 202: // Error condition for now. Complex tables are not yet supported > This is still putting all SqlCalls into currentlyVisitedSqlNodes_. In patch set 4, at https://gerrit.cloudera.org/c/23209/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java#221:~:text=(SqlCall%20call)%20%7B-,if%20(call%20instanceof%20SqlWith)%20%7B,%7D,-if%20(call.getKind I had added an if block to only add SqlWith nodes. > So do we need to put anything else in the LinkedList besides "SqlWith" nodes? No we don't. You are correct. > This would also make "findClosestSqlWith()" much easier, since it would > always be the one most recently entered in the LinkedList (you probably > wouldn't even need a method for it) I agree with you. http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@211 PS3, Line 211: // This map associates each SqlWith node with the TableName's registered by its > Nah, not concerned about memory at all. Just didn't know how it worked. C Ack http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@249 PS3, Line 249: > Same comment as above: Is this needed? No, it seems that it is not needed. Based on my observation, it looks like 'fromNode' won't be a SqlWith node. I will remove this call to add() the the corresponding removeLast() in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/23209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f51af42d64cdcff3c26ad5a96c7f53ebef431b3 Gerrit-Change-Number: 23209 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Thu, 04 Sep 2025 02:23:01 +0000 Gerrit-HasComments: Yes
