Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24211 )
Change subject: IMPALA-14909: Calcite planner: Fix validation for views with column list ...................................................................... Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/24211/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java: http://gerrit.cloudera.org:8080/#/c/24211/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java@281 PS5, Line 281: validator.startValidatingView(); > I don't think we should fix the view string in HMS. That's a bug/behavior o Ah, I see what you mean. Heh, yeah, I didn't think the HMS solution was viable. It's been awhile since I code this, but to be honest? I think my mentality was to treat this as a two pass hack for safety reasons. And here's what I remember, I think... If everything is ok, then we rely on non-hacky code. Sure, there is extra processing, but it's just tracking information. If it's broken, then we can do our best to fix it. I suppose because of its hacky nature, there's some possibility I didn't quite get it right? I think it's good, but to say I'm 100% sure...well, I never like to say 100%. But on the off-chance it's still broken? Then it will fallback to the original planner. So if I fix this in a one pass solution, I am changing the code path of the working solution, and I didn't feel comfortable with that. I'm still not sure I do. http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java: http://gerrit.cloudera.org:8080/#/c/24211/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java@288 PS6, Line 288: parsedSqlNode = validator.validate(parsedSqlNode); > What confused me is how we handle nested views that also have this alias is That's a good question. It's not quite obvious, eh? This response is gonna be a bit long. So there is some recursion. The recursion exists at line 258. So it will validate every view (and check for permissions/privileges of the user). However: The validation of each individual view is isolated and there is no recursion while it is calling the Calcite validation step and ensuring that the SQL is valid. When it does the Calcite validation, it needs to ensure that the select columns match the columns created in the from clause which contains the table/view. The table/view is stored in their CalciteCatalogReader. The CalciteCatalogReader is populated by the StmtTableCache which is also used by the original planner. The StmtTableCache gets populated by its "loadTables()" method which is only called for the top level. But this loadTables() method does recursion on its views and fetches all the FeTable and FeView objects so they are all known by the time we hit this part of the code. So yeah, at this point, the problem only is within the FeView sql itself. It's an internal alias for the query that is a problem. If there is a nested view, we don't have to worry about it because there is no problem with the names coming out of the view from an external point of view. I hope that makes sense and is coherent I had to do several edits here along the way and some verification through the debugger as well. If not, I'll try again, just let me know. -- To view, visit http://gerrit.cloudera.org:8080/24211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0d0229c8907f69e648034d12fe375d9d9a384e25 Gerrit-Change-Number: 24211 Gerrit-PatchSet: 6 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 08 Jun 2026 15:50:53 +0000 Gerrit-HasComments: Yes
