Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8958 )
Change subject: IMPALA-5152: Introduce metadata loading phase ...................................................................... Patch Set 5: (5 comments) Minor comments left http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@519 PS5, Line 519: analysisResult_.isDescribeTableStmt() || : analysisResult_.isResetMetadataStmt() || : analysisResult_.isUseStmt() || : analysisResult_.isShowTablesStmt() || : analysisResult_.isAlterTableStmt()) nit: maybe create a function like you did for isHierarchicalAuthStmt(). http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@104 PS5, Line 104: reset() Haven't seen where this is called yet, but I am wondering if we also need to clear the timeline as well. http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/catalog/Catalog.java@154 PS5, Line 154: Throws if the database : * does not exists. Returns null if the table does not exist. Isn't this kind of weird behavior? throw in one case and return null in the other? Is it easy to simplify it? If not, mind adding a TODO to clean this a bit in the future. http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/service/Frontend.java@956 PS5, Line 956: , nit: remove http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/service/Frontend.java@962 PS5, Line 962: , nit: remove -- To view, visit http://gerrit.cloudera.org:8080/8958 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I68d32d5acd4a6f6bc6cedb05e6cc5cf604d24a55 Gerrit-Change-Number: 8958 Gerrit-PatchSet: 5 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 20 Feb 2018 21:22:55 +0000 Gerrit-HasComments: Yes
