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: (3 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@202 PS3, Line 202: currentlyVisitedSqlNodes.add(call); > Done This is still putting all SqlCalls into currentlyVisitedSqlNodes_. I guess I was thinking is this (and again, perhaps I'm missing something): I only saw the variable "currentlyVisitedSqlNodes_" examined within methods "findClosestSqlWith()" and "isRegisteredBySqlItem()". In both of these methods, it only searches for SqlNodes of type "SqlWith". So do we need to put anything else in the LinkedList besides "SqlWith" nodes? Of course, the "removeLast()" in this method would need to be tracked with a boolean if added, but that's easy enough. 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) 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) { > > I can see that a TableName object contains a lot of fields that are not r Nah, not concerned about memory at all. Just didn't know how it worked. Code seems fine (but see comment above about fineClosestSqlWith(). ) 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: currentlyVisitedSqlNodes.add(fromNode); Same comment as above: Is this needed? -- 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 22:28:46 +0000 Gerrit-HasComments: Yes
