Steve Carlin 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) 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<>(); nit: member names for Impala have "_" at the end Also optional (if the code stays as/is with regards to some of my other comments): Would a "Stack" be more appropriate? 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. 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 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 "currentlyVisitedSqlNodes" that we care about are of type SqlWith. Can we just put "SqlWith" types into currentlyVisitedSqlNodes rather than every SqlCall? 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? That's the thing I would imagine we want to avoid putting in the tablenames. Is there ever more than one? 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. But if you want to get cute about it, you can handle these lines of code (where the set doesn't exist in the hashmap) in one line as done here: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java#L1193 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)" rather than using an iterator? -- 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: Thu, 28 Aug 2025 20:39:31 +0000 Gerrit-HasComments: Yes
