Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 )
Change subject: IMPALA-3268: Add support for SHOW VIEWS statement ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/20669/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/20669/5/be/src/service/client-request-state.cc@366 PS5, Line 366: const std::set<TImpalaTableType::type>* table_types = &(params->table_types); > Thanks for catching this Michael! nit: I'd probably do const std::set<TImpalaTableType::type>& table_types = params->table_types; RETURN_IF_ERROR(frontend_->GetTableNames(params->db, table_name, &query_ctx_.session, &table_types, &table_names)); as it leaves table_types as a simple alias and groups dereferencing in the function call. The current version is fine too. http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.h@65 PS7, Line 65: Status GetTableNames(const std::string& db, const std::string* pattern, I'm not really sure why this was split out as a separate function signature rather than relying on the empty table_types default value. Although with my other suggestion of taking table_types by ref, it probably makes sense for it to be a separate function because we wouldn't want to append it to the end (since we use that for returning the list of table_names). http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/20669/7/be/src/service/frontend.cc@210 PS7, Line 210: params.__set_table_types(*table_types); If table_types is null, this crashes. We should take table_types as a ref rather than a pointer, since it can't be changed and the default value is an empty set. -- To view, visit http://gerrit.cloudera.org:8080/20669 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I321fc5350392a815949a4e7d2a64d60466689788 Gerrit-Change-Number: 20669 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 28 Nov 2023 00:28:13 +0000 Gerrit-HasComments: Yes
