Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles. ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11387/3/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/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1065 PS3, Line 1065: if (!analysisResult.isCreateTableAsSelectStmt()) { : return result; : } I personally think it's weird, but the impala style is to use single-line if statements for stuff like this http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1134 PS3, Line 1134: private void setMtDopForCatalogOp(AnalysisResult analysisResult, can be static? same with several of the extracted functions below http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1150 PS3, Line 1150: createTExecRequest maybe rename this to 'createBaseExecRequest' or something? Otherwise it's sort of weird that we have 'createExecRequest' and 'createTExecRequest' and it's not quite clear why one does way more than the others. http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1164 PS3, Line 1164: AnalysisResult analysisResult, it seems this is just used to get the insertStmt, and the insertStmt is already available at the call site. Just take the InsertStmt here directly so it has narrower "scope" (it's more obvious at call sites that this only depends on the insert stmt result) http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1185 PS3, Line 1185: void addQueryMetadata it seems that 'result' is only used as an argument here for storing the final created TResultSetMetadata. Could you instead return TResultSetMetadata and have the call site look like result.setResult_set_metadata(createQueryResultSetMetadata(analysisResult)) or somesuch? Style-wise I think that's preferred since the function then becomes "pure" (side-effect-free) which is kinda nice. It also seems this method could be static, which makes it more obvious that there are no side-effects. -- To view, visit http://gerrit.cloudera.org:8080/11387 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5 Gerrit-Change-Number: 11387 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 07 Sep 2018 17:33:15 +0000 Gerrit-HasComments: Yes
