Yu-Wen Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18626 )
Change subject: IMPALA-9670: Fix unloaded views are shown as tables for GET_TABLES requests ...................................................................... Patch Set 3: (4 comments) The patch looks good to me in general! Just left some question and minor comments. http://gerrit.cloudera.org:8080/#/c/18626/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/18626/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@46 PS3, Line 46: import org.apache.hadoop.hive.metastore.TableType; Maybe TableType can be removed as well? http://gerrit.cloudera.org:8080/#/c/18626/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/18626/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1903 PS3, Line 1903: String tableName = tblMeta.getTableName().toLowerCase(); Could you confirm if converting tableName to lower case is OK? I ask just because it was not converted to lower case before. If it's OK, we probably don't need to call toLowerCase next line. http://gerrit.cloudera.org:8080/#/c/18626/3/fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java File fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java: http://gerrit.cloudera.org:8080/#/c/18626/3/fe/src/main/java/org/apache/impala/catalog/Hive3MetastoreShimBase.java@290 PS3, Line 290: case MATERIALIZED_VIEW: I figure I should ask the question here because sometimes we treat materialized view as table. Does it matter in the context of this patch? http://gerrit.cloudera.org:8080/#/c/18626/3/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/18626/3/tests/common/impala_test_suite.py@119 PS3, Line 119: str(IMPALAD_BEESWAX_PORT_LIST[i] - IMPALAD_BEESWAX_PORT + IMPALAD_HS2_PORT) I might missed something. Could you explain why we need to calculate port here? (same question for line 127) -- To view, visit http://gerrit.cloudera.org:8080/18626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I528bb20272ebdd66a0118c30efc2b0566f2b0e2f Gerrit-Change-Number: 18626 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Comment-Date: Fri, 17 Jun 2022 23:47:08 +0000 Gerrit-HasComments: Yes
