Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20669 )
Change subject: IMPALA-3268: Add support for SHOW VIEWS statement ...................................................................... Patch Set 6: (3 comments) Hi all, I slightly revised the patch set 5 to address some of Michael's comments. Let me know if there are still additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/20669/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-3268: Add support for SHOW VIEWS statement > The ticket mentions having a way to get both tables and views in a single c Thanks Michael! This patch does not change the current behavior of the SHOW TABLES statement. I will emphasize this in the commit message in the next patch. Specifically, for SHOW VIEWS, we explicitly set up the table types to retrieve at https://gerrit.cloudera.org/c/20669/5/fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java#54. When we do not set up the table types to match, this field defaults to an empty set. For an empty set, we return all types of tables (refer to https://gerrit.cloudera.org/c/20669/5/fe/src/main/java/org/apache/impala/catalog/Db.java#224). I moved the following items out of the scope of IMPALA-3268 to a newly created one: IMPALA-12574 because we may need more discussion around whether we need to consider materialized views as tables or views. Currently SHOW TABLES in Cloudera's distribution of Hive returns both tables, views, and materialized views and SHOW VIEWS does not return materialized views (refer to https://gerrit.cloudera.org/c/20669/5/testdata/workloads/functional-query/queries/QueryTest/show_views.test#92). - SHOW TABLES should only return tables. - add a flag to either above commands to return all tables and views. 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); > Can we take a ref of table_types? I don't see any reason to make a copy her Thanks for catching this Michael! Yes, we can pass the pointer to the set of table types to GetTableNames() instead. I will do this in the next patch. I will also reorder the arguments of GetTableNames() so that the returned 'table_names' will still be the last argument after this patch since we seem to always have the returned result as the last argument for functions in the class of Frontend. http://gerrit.cloudera.org:8080/#/c/20669/5/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/20669/5/common/thrift/CatalogService.thrift@530 PS5, Line 530: UNKNOWN, > UNKNOWN seems like it should come after other types. Do we rely on that any Thanks Michael! I put the new type as the last one because I saw in IMPALA-8234 (https://gerrit.cloudera.org/c/12543/8/common/thrift/Metrics.thrift#49) that we reverted what we did in IMPALA-7694, where we inserted a field in the middle of the Metrics.TUnit enum (https://gerrit.cloudera.org/c/12069/22/common/thrift/Metrics.thrift#32). According to this, maybe it is better we always add the new field as the last one for backward compatibility? -- 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: 6 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Thu, 23 Nov 2023 19:12:20 +0000 Gerrit-HasComments: Yes
