Alex Behm 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) 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(). Done 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 t Resetting the timeline does not make sense because it may already contain events. To avoid confusion, I removed reset() and added comments and Preconditions checks to indicate loadTables() should only be called once per statement. 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 Added TODO. Note that the previous getTable() had a bug because if you passed !throwIfError then an NPE could be hit. I think this new version is at least better than before, albeit still weird. 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 Done http://gerrit.cloudera.org:8080/#/c/8958/5/fe/src/main/java/org/apache/impala/service/Frontend.java@962 PS5, Line 962: , > nit: remove Done -- 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: Wed, 21 Feb 2018 06:39:26 +0000 Gerrit-HasComments: Yes
