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 6: (22 comments) http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@13 PS6, Line 13: table's comment can be displayed if catalog table : owns Hive metastore table(HMS) object There is no notion of ownership here. A loaded table is always associated with the an HMS table object. So, you should say that a table's comment (if any) is displayed if the table is loaded. http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@14 PS6, Line 14: There is not any HMS call : due to performance concern, so table's comment can be empty even : though table has a comment. Rewrite as: "If the table is not loaded, an empty comment is returned." http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@18 PS6, Line 18: There is a change in thrift part: a list of table comments is added to : TGetTablesResult struct as an optional element. Remove this sentence. http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc@381 PS6, Line 381: for (const TTableInfo& table_info: get_tables_info_result.tableinfos) { : Value table_obj(kObjectType); : Value fq_name(Substitute("$0.$1", db.db_name, table_info.tbl_name).c_str(), : document->GetAllocator()); : table_obj.AddMember("fqtn", fq_name, document->GetAllocator()); : Value table_name(table_info.tbl_name.c_str(), document->GetAllocator()); : table_obj.AddMember("name", table_name, document->GetAllocator()); : table_array.PushBack(table_obj, document->GetAllocator()); : } Why not returning the table comments as well? http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h File be/src/catalog/catalog.h: http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h@83 PS6, Line 83: Hive metastore was not loaded for a table "table is not loaded". http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc@140 PS6, Line 140: return JniUtil::CallJniMethod(catalog_, get_table_names_id_, params, tables_info_result); long line http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc@275 PS6, Line 275: for (const TTableInfo& tbl_info: tables_info_result.tableinfos) { : names.push_back(tbl_info.tbl_name); : comments.push_back(tbl_info.comment); : } : SetResultSet(names, comments); This code will result to DCHECK being hit if some table doesn't have the comment set. http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc@139 PS6, Line 139: Status Frontend::GetTableNames(const string& db, const string* pattern, : const TSessionState* session, TGetTablesInfoResult* tables_info_result) { : TGetTablesInfoParams params; : params.__set_db(db); : if (pattern != NULL) params.__set_pattern(*pattern); : if (session != NULL) params.__set_session(*session); : return JniUtil::CallJniMethod(fe_, get_table_names_id_, params, tables_info_result); : } : : Status Frontend::GetTableNamesAndComments(const string& db, const string* pattern, : const TSessionState* session, TGetTablesInfoResult* tables_info_result) { : TGetTablesInfoParams params; : params.__set_db(db); : if (pattern != NULL) params.__set_pattern(*pattern); : if (session != NULL) params.__set_session(*session); : return JniUtil::CallJniMethod(fe_, get_table_names_and_comments_id_, params, : tables_info_result); : } Should be a single call Frontend::GetTablesInfo() http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc@522 PS6, Line 522: for (const TTableInfo& tbl_info: get_tables_info_result.tableinfos) { : Value table_obj(kObjectType); : Value fq_name(Substitute("$0.$1", db.db_name, tbl_info.tbl_name).c_str(), : document->GetAllocator()); : table_obj.AddMember("fqtn", fq_name, document->GetAllocator()); : Value table_name(tbl_info.tbl_name.c_str(), document->GetAllocator()); : table_obj.AddMember("name", table_name, document->GetAllocator()); : table_array.PushBack(table_obj, document->GetAllocator()); : } Populate the comment, if it's there. http://gerrit.cloudera.org:8080/#/c/8851/6/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/8851/6/common/thrift/Frontend.thrift@85 PS6, Line 85: a result of show table for a table information about a table including table name and optionally, a table comment. http://gerrit.cloudera.org:8080/#/c/8851/6/common/thrift/Frontend.thrift@90 PS6, Line 90: May not be set if the database metastore was not loaded. 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. 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@406 PS6, Line 406: / Not found comment if metastore table is a view remove comment http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java@407 PS6, Line 407: final remove http://gerrit.cloudera.org:8080/#/c/8851/6/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/8851/6/fe/src/main/java/org/apache/impala/service/Frontend.java@624 PS6, Line 624: tables table names http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java@167 PS6, Line 167: thriftGetTablesInfoParams This name doesn't make sense. Can you change it to thriftGetDbsParams? http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java@191 PS6, Line 191: tableinfos nit: tablesInfo http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java@193 PS6, Line 193: tableinfo tableInfo (camel-case) http://gerrit.cloudera.org:8080/#/c/8851/6/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/6/fe/src/main/java/org/apache/impala/service/JniFrontend.java@45 PS6, Line 45: import org.apache.impala.catalog.MetaStoreClientPool.MetaStoreClient; Do you still need this? http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/JniFrontend.java@253 PS6, Line 253: public byte[] getTableNames(byte[] thriftGetTablesInfoParams) throws ImpalaException { : return getTableNamesAndComments(thriftGetTablesInfoParams, false); : } : : /** : * Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] 'pattern']", and : * return a list of table names and table comments matching an optional pattern. : * The only metacharacters are '*' which matches any string of characters, and '|' : * which denotes choice. Doing the work here saves loading tables or databases from the : * metastore (which Hive would do if we passed the call through to the metastore : * client). If the pattern is null, all strings are considered to match. If it is an : * empty string, no strings match. : * : * 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. : * : * The argument is a serialized TGetTablesInfoParams object. : * The return type is a serialised TGetTablesInfoResult object. : */ : public byte[] getTableNamesAndComments(byte[] thriftGetTablesInfoParams) : throws ImpalaException { : return getTableNamesAndComments(thriftGetTablesInfoParams, true); : } : : /** : * Internal method to return a list of table names and table comments : * matching on optional pattern. : * : * @see Frontend#getTables : */ : private byte[] getTableNamesAndComments(byte[] thriftGetTablesInfoParams, : boolean collectComments) throws ImpalaException { : TGetTablesInfoParams params = new TGetTablesInfoParams(); : JniUtil.deserializeThrift(protocolFactory_, params, thriftGetTablesInfoParams); : // If the session was not set it indicates this is an internal Impala call. : User user = params.isSetSession() ? : new User(TSessionStateUtil.getEffectiveUser(params.getSession())) : : ImpalaInternalAdminUser.getInstance(); : : Preconditions.checkState(!params.isSetSession() || user != null ); : List<Table> tables = frontend_.getTables(params.db, : PatternMatcher.createHivePatternMatcher(params.pattern), user); : : TGetTablesInfoResult result = new TGetTablesInfoResult(); : List<TTableInfo> tableinfos = Lists.newArrayListWithCapacity(tables.size()); : for (Table table: tables) { : TTableInfo tableinfo = new TTableInfo(); : tableinfo.setTbl_name(table.getName()); : if (collectComments) tableinfo.setComment(table.getComment()); : tableinfos.add(tableinfo); : } : result.setTableinfos(tableinfos); : : TSerializer serializer = new TSerializer(protocolFactory_); : try { : return serializer.serialize(result); : } catch (TException e) { : throw new InternalException(e.getMessage()); : } : } Replace with single getTablesInfo() call. If you want to control whether comments are returned or not you can add a param in TGetTablesInfoParams, but I don't see why not always returning the comment. http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/MetadataOp.java File fe/src/main/java/org/apache/impala/service/MetadataOp.java: http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/MetadataOp.java@267 PS6, Line 267: ImpaladCatalog catalog = fe.getCatalog(); Do you still need that? http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/service/MetadataOp.java@280 PS6, Line 280: final no need for that http://gerrit.cloudera.org:8080/#/c/8851/6/tests/query_test/test_queries.py File tests/query_test/test_queries.py: http://gerrit.cloudera.org:8080/#/c/8851/6/tests/query_test/test_queries.py@214 PS6, Line 214: class TestShowTables(TestQueries): : @classmethod : def get_workload(cls): : return 'functional-query' : : def testTableCommentVisibility(self, unique_database): : self.execute_query("create table {db}.t1(a int) comment 'bla bla bla'".format( : db=unique_database)) : self.execute_query("select * from {db}.t1".format(db=unique_database)) : result = self.execute_query("show tables in {db}".format(db=unique_database)) : assert "bla bla bla" in result.data[0] Tests for this should be in test_metadata_query_statements.py (see show.test) -- 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: 6 Gerrit-Owner: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Comment-Date: Wed, 17 Jan 2018 01:27:23 +0000 Gerrit-HasComments: Yes
