Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14422 )
Change subject: IMPALA-8738: Extend "show tables" to return values other than the table name ...................................................................... Patch Set 1: (14 comments) Thanks so much for your contribution! It mostly makes sense, but I have a few suggestions. One issue is that you have a large number of formatting errors, eg. incorrect use of whitespace. Rather than me going through and pointing each individual one out, it would save us both a lot of time if you would look into clang-format-diff as described on this page: https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala. which will automatically do the formatting for you. Some notes about clang-format-diff: only reformat lines that you're actually changing, not just everything in the changed files. Also, we do sometimes deviate from what clang-format says to do, so if it tells you to do something that's clearly different than the way other similar things are formatted you can feel free to ignore those suggestions. http://gerrit.cloudera.org:8080/#/c/14422/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14422/1//COMMIT_MSG@9 PS1, Line 9: nit: remove all of the extra whitespace to the left of all of these lines http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/client-request-state.cc@323 PS1, Line 323: table_names This name is a little weird now that it could contain things other than table names. How about renaming this 'result' http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/frontend.h@62 PS1, Line 62: SHOW TABLES SHOW [EXTENDED] TABLES http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/frontend.h@73 PS1, Line 73: & remove this, no need to pass bools by reference http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/14422/1/be/src/service/impala-http-handler.cc@591 PS1, Line 591: is_extend instead of declaring the variable only to use it once, you can remove the line above and replace this with "/* isExtended */ false" http://gerrit.cloudera.org:8080/#/c/14422/1/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/14422/1/common/thrift/Frontend.thrift@84 PS1, Line 84: // True if return information other than the name : // Default is false. If not set, only table name is returned Replace all of this with: True if this was a SHOW EXTENDED http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/cup/sql-parser.cup@2472 PS1, Line 2472: |KW_SHOW KW_EXTENDED KW_TABLES : {: RESULT = new ShowTablesStmt(true); :} : | KW_SHOW KW_EXTENDED KW_TABLES show_pattern:showPattern : {: RESULT = new ShowTablesStmt(showPattern,true); :} : | KW_SHOW KW_EXTENDED KW_TABLES KW_IN ident_or_default:db : {: RESULT = new ShowTablesStmt(db, null,true); :} : | KW_SHOW KW_EXTENDED KW_TABLES KW_IN ident_or_default:db show_pattern:showPattern : {: RESULT = new ShowTablesStmt(db, showPattern,true); :} I think that we can make this a lot simpler by adding a new rule 'opt_extended', which would look something like: opt_extended ::= KW_EXTENDED {: RESULT = true; :} | /* empty */ {: RESULT = false; :} ; Then instead of copying all of the existing rules, you can modify them to use this, eg. the first rule would become something like: KW_SHOW opt_extended:extended KW_TABLES {: RESULT = new ShowTablesStmt(extended); :} http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java: http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java@21 PS1, Line 21: import org.apache.impala.authorization.User; remove this, it isn't used anywhere http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java@23 PS1, Line 23: import org.apache.impala.common.InternalException; remove this, it isn't used anywhere http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java@52 PS1, Line 52: // If false,only name is returned; Otherwise return name and table_type Lets drop this line from here and instead mention in the class comment at the top that if EXTENDED is specified then the table type is returned. http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java@64 PS1, Line 64: public ShowTablesStmt(boolean extended) { Instead of adding a bunch of new constructors, lets just add 'extended' as a parameter to all of the existing constructors. Then, you'll just need to pass in 'false' for 'extended' anywhere we currently use those constructors, which I believe is just in sql-parser.cup http://gerrit.cloudera.org:8080/#/c/14422/1/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/14422/1/fe/src/main/java/org/apache/impala/service/Frontend.java@a773 PS1, Line 773: Might as well leave this as it was to avoid the extra call to getCatalog() http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/service/Frontend.java@780 PS1, Line 780: is_extended isExtended http://gerrit.cloudera.org:8080/#/c/14422/1/fe/src/main/java/org/apache/impala/service/Frontend.java@820 PS1, Line 820: .deepCopy() Why is this necessary? -- To view, visit http://gerrit.cloudera.org:8080/14422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63057c9d2fc453f95c6890bdc90e11c61a98a419 Gerrit-Change-Number: 14422 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward <cnsdjnfan...@qq.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Thu, 17 Oct 2019 23:25:46 +0000 Gerrit-HasComments: Yes
