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 7: (10 comments) Hi all, I have revised the patch set 6 according to the comments from Quanlong. Let me know if there are additional suggestions. Thanks very much for the help! http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG@13 PS6, Line 13: y. > nit: "catalog server" Done http://gerrit.cloudera.org:8080/#/c/20669/6//COMMIT_MSG@14 PS6, Line 14: > Is this feature depends on this flag? I think if the views are loaded, they Thanks Quanlong! Yes. This feature depends on this flag. I will emphasize this in the commit message of the next patch. Specifically, if we start the catalog server without this flag, then Impala could not determine whether or not a given Table is a view without a user explicitly executing statements like REFRESH. In addition, the table metadata would be lost after the INVALIDATE METADATA statement. This could be shown in the following sequence of queries. On the other hand, if we start the catalog server with this flag, SHOW VIEWS always returns those tables whose getTableType() evaluate to TImpalaTableType.VIEW. [localhost:21050] default> show views in functional; Query: show views in functional Fetched 0 row(s) in 0.04s [localhost:21050] default> refresh functional.alltypes_view; Query: refresh functional.alltypes_view Query submitted at: 2023-11-27 11:41:34 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=4d4c7564e5a13fef:f3d8c13100000000 [localhost:21050] default> show views in functional; Query: show views in functional +---------------+ | name | +---------------+ | alltypes_view | +---------------+ Fetched 1 row(s) in 0.00s [localhost:21050] default> invalidate metadata functional.alltypes_view; Query: invalidate metadata functional.alltypes_view Query submitted at: 2023-11-27 11:42:07 (Coordinator: http://fangyu-upstream-dev.gce.cloudera.com:25000) Query state can be monitored at: http://fangyu-upstream-dev.gce.cloudera.com:25000/query_plan?query_id=6b4c9396e5147b00:eeeba56600000000 [localhost:21050] default> show views in functional; Query: show views in functional Fetched 0 row(s) in 0.00s http://gerrit.cloudera.org:8080/#/c/20669/6/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/20669/6/be/src/service/frontend.cc@199 PS6, Line 199: strin > nit: we don't need explicitly using the "std" namespace for std::string and Thanks Quanlong! I will remove the namespace in the next patch. http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/analysis/ShowViewsStmt.java@54 PS6, Line 54: > nit: we can use com.google.common.collect.Sets directly as Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@241 PS6, Line 241: Collections.emp > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Db.java@208 PS6, Line 208: > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/Db.java@220 PS6, Line 220: lect(Collectors.toList()); > nit: This doesn't leverage the fact that 'tableTypes' is a set. I think it Thanks Quanlong! I will revise this in the next patch. http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@124 PS6, Line 124: Collections.emp > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java: http://gerrit.cloudera.org:8080/#/c/20669/6/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@162 PS6, Line 162: > nit: use Collections.emptySet() to avoid creating new objects. Done http://gerrit.cloudera.org:8080/#/c/20669/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/20669/6/fe/src/main/java/org/apache/impala/service/Frontend.java@1141 PS6, Line 1141: Collections.emp > nit: use Collections.emptySet() to avoid creating new objects. Done -- 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: Mon, 27 Nov 2023 23:46:59 +0000 Gerrit-HasComments: Yes
