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 3: (6 comments) I replied to the questions from Steve and will add more tests in my next patch set. 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: public LinkedList<SqlNode> currentlyVisitedSqlNodes = new LinkedList<>(); > I have mixed feelings on that because a Stack seems more natural with > "removeLast", but you also iterate through the Stack, and usually that isn't > done with Stacks. I'm ok either way. Yeah. Since we also iterate through the stack, I will keep the implementation as is. http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@194 PS3, Line 194: public final Map<SqlWith, Set<TableName>> withItemsMap = new HashMap<>(); > same as above Ack 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: currentlyVisitedSqlNodes.add(call); > On my scan through the code here (and perhaps I missed something), the only Yes. It is a good idea. It may be enough for us to put only "SqlWith" nodes into currentlyVisitedSqlNodes. 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: if (call.getKind() == SqlKind.WITH_ITEM) { > A general question: In the SQL like "WITH t1 as (select blah blah blah...), > is the WITH_ITEM "t1" here? Yes. "t1" would correspond to exactly one SqlWithItem here. > That's the thing I would imagine we want to avoid putting in the tablenames. > Is there ever more than one? A SqlWith node could have more than one SqlWithItem's. For instance, in the following query, the SqlWith node would have 2 SqlWithItem's corresponding to "t1" and "t2", respectively. with t1 as (select id from functional.alltypestiny), t2 as (select id from functional.alltypestiny) select * from t1, t2; Not very sure if I understand you correctly, do you want us to just put in a string or a pair of string for the visited SqlWithItem? I can see that a TableName object contains a lot of fields that are not really needed. We only need the database name and the table name for the visited SqlWithItem in our case. I could make the change accordingly if my understanding is correct. http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@218 PS3, Line 218: Set<TableName> tableNames = withItemsMap.get(closestSqlWith); > Optional: I don't mind this code and it might be more readable this way. B Thanks Steve! Yeah. Using computeIfAbsent() would make the code much more concise. I will use this method in the next patch. http://gerrit.cloudera.org:8080/#/c/23209/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@292 PS3, Line 292: Iterator<SqlNode> reverseIterator = currentlyVisitedSqlNodes.descendingIterator(); > Optional: can we just use "for (SqlNode node : currentlyVisitedSqlNodes)" r Yes. I will make the corresponding change 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: 3 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: Wed, 03 Sep 2025 17:30:42 +0000 Gerrit-HasComments: Yes
