Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8851 )
Change subject: IMPALA-3193: Show table's comment on show tables ...................................................................... Patch Set 7: (12 comments) Applied update. http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h File be/src/catalog/catalog.h: http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h@79 PS7, Line 79: Returns all matching table names and table comments, per Hive's : /// "SHOW TABLES <pattern>". Each table name of TTableInfo returned is unqualified. : /// Comment cannot be set when table is not loaded. If pattern is NULL, match all : /// tables otherwise match only those tables that match the pattern string. Patterns : /// are "p1|p2|p3" where | denotes choice, and each pN may contain wildcards denoted : /// by '*' which match all strings. Table comments can be empty if table is not loaded. > Let's fix this comment a bit: Done http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc@280 PS7, Line 280: if (!tbl_info.__isset.comment) DCHECK(tbl_info.comment.empty()); : comments.push_back(tbl_info.comment); > This check is a bit confusing. Ideally, you want to ensure that names and c Sorry, actually I didn't catch your intention for adding DCHECK in the previous comment. I still don't understand your last sentence. Why should we consider this(i.e. check)? As far as I understand, comment should be either empty(table is not loaded case or a given empty string as a comment) or not empty. Hence, we just put it to comments. Anyway, we should guarantee that the number of names should be equal to the number of comments. Let me remove the DCHECK code line which seems to be meaningless now. http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h@57 PS7, Line 57: /// Returns all matching table names and table comments. : /// If pattern is NULL, match all tables otherwise match only those tables that : /// match the pattern string. Patterns are "p1|p2|p3" where | denotes choice, : /// and each pN may contain wildcards denoted by '*' which match all strings. : /// The TSessionState parameter is used to filter results of metadata operations when : /// authorization is enabled. If this is a user initiated request, it should : /// be set to the user's current session. If this is an Impala internal request, : /// the session should be set to NULL which will skip privilege checks returning all : /// results. Table comments can be empty if table is not loaded. > See previous comment about this comment :). Done http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc@145 PS7, Line 145: return JniUtil::CallJniMethod(fe_, get_tables_info_id_, params, : tables_info_result); > nit: check if it fits in a single line Done http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@91 PS7, Line 91: Comment needs to indicate when and if this is set. For instance, : // it's not clear if this is set if the table is loaded but there is no comment. > I believe this was my comment. The point was to explicitly specify the beha Done. Sorry, it's my mistake. I was confusing table 'comment' and your 'comment'. http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@96 PS7, Line 96: // getTablesInfo returns a list of table names and a list of table comment. > comments is not consistent with the struct definition, plz update Done http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@98 PS7, Line 98: tableinfos > tables_info Done http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java@407 PS6, Line 407: blic > My thinking is that we're not particularly consistent with using final in o Thanks for sharing your idea. It's just my curiosity. I want to know the reason why you thought. Actually I didn't find the use of final keyword elsewhere except for class member variable. I would like to follow other authors' preference. http://gerrit.cloudera.org:8080/#/c/8851/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/8851/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@249 PS7, Line 249: * Note: There is a cost to get a comment because a communication happens between : * Impala and Hive's metastore at a first time. In the first request, the metastore : * table is cached. > comment no longer valid, plz remove Done http://gerrit.cloudera.org:8080/#/c/8851/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@315 PS7, Line 315: thriftGetTablesInfoParams > name doesn't match the function context. Why would I pass a thriftGetTableI Done http://gerrit.cloudera.org:8080/#/c/8851/7/tests/metadata/test_metadata_query_statements.py File tests/metadata/test_metadata_query_statements.py: http://gerrit.cloudera.org:8080/#/c/8851/7/tests/metadata/test_metadata_query_statements.py@181 PS7, Line 181: class TestShowTables(ImpalaTestSuite): > Are you sure the tests in show.test are not affected by this patch? Sorry, I missed what you mentioned in the previous comment. Done. http://gerrit.cloudera.org:8080/#/c/8851/7/tests/metadata/test_metadata_query_statements.py@186 PS7, Line 186: testTableCommentVisibility > Let's make this test a bit more interesting. Create at least another table Done -- To view, visit http://gerrit.cloudera.org:8080/8851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd Gerrit-Change-Number: 8851 Gerrit-PatchSet: 7 Gerrit-Owner: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Comment-Date: Wed, 24 Jan 2018 03:00:54 +0000 Gerrit-HasComments: Yes
