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 6: (22 comments) Applied the update. 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 w Done 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." Done 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. Done 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? Done 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". Done 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 Done 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 co Done 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() Done 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. Done 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 comm Done 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 c 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@406 PS6, Line 406: / Not found comment if metastore table is a view > remove comment Done http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java@407 PS6, Line 407: final > remove Done. By the way, may I know why you would not prefer final keyword here? I usually prefer to use const in C++ and final in Java to represent the variable should be immutable. 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 Done 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? Done 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 Done 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) Done 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? Done 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 co Done 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? Done 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 Done 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.tes 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: 6 Gerrit-Owner: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Comment-Date: Tue, 23 Jan 2018 15:03:57 +0000 Gerrit-HasComments: Yes
