Dimitris Tsirogiannis 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) 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: "Returns information of all matching tables in 'db' based on the input 'pattern'. If pattern is NULL, return information of all tables in 'db' otherwise return information of only those tables whose names 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 information includes the fully qualified table name and, optionally, the table's comment, if set. Table comment will be empty if a table is not loaded." 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 comments will have the same number of elements. So, either check that comment is always set or set it yourself to an empty string and put it to 'comments'. 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 :). 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 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 behavior re setting the comment field. 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 http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@98 PS7, Line 98: tableinfos tables_info 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 > Done. By the way, may I know why you would not prefer final keyword here? I My thinking is that we're not particularly consistent with using final in our java-based codebase and we typically don't use it unless we absolutely have to. I see how it can help in some cases. So, you may keep it if you want. 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 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 thriftGetTableInfoParams input param to a getDbs call? 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? 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 with comments and one without and make sure show tables returns the correct results. -- 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: Tue, 23 Jan 2018 20:36:45 +0000 Gerrit-HasComments: Yes
