Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 )
Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/13375/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13375/3//COMMIT_MSG@10 PS3, Line 10: when integration with Hive Metastore is enabled > nit: when integration with Hive Metastore is enabled Done http://gerrit.cloudera.org:8080/#/c/13375/3//COMMIT_MSG@11 PS3, Line 11: for CREATE TABLE statement, Impala can : rely on Kudu to create the table in the HMS. > nit: consider rewriting it to be easier to parse Done http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@117 PS3, Line 117: super(msTable, db, name, owner); : kuduTableName_ = msTable.getParameters().get(KuduTable.KEY_TABLE_NAME); : kuduMasters_ = msTable.getParameters().get(KuduTable.KEY_MASTER_HOSTS); : if (kuduTableName_ == null || kuduTableName_.isEmpty()) { : // When 'kudu.table_name' property is emp > Would it make sense to create a method/function that will hide these implem Done http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@260 PS3, Line 260: // This is set to 0 for Kudu tables. : // TODO: Change this to reflect the number of pk columns and modify all the : // places (e.g. insert stmt) that currently make use of this parameter. : numClusteringCols_ = 0; : org.apache.kudu.client.KuduTable kuduTable = > ditto -- would it help to create a function/method and then re-use it in su Done http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1893 PS3, Line 1893: * For managed tables: : * 1. If Kudu's integration with the Hive Metastore is not enabled, the Kudu : * table is first created in Kudu, then in the HMS. : * 2. Otherwise, when the table is created in Kudu, we rely on Kudu to have : * created the table in the HMS. : * For external tables: : * 1. We only create the table in the HMS (regardless of Kudu's integration : * with the Hive Metastore). : * > nit: the structure of this felt a bit strange since there are three non-par Thanks for the suggestion! Updated. http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1909 PS3, Line 1909: Preconditions.checkState(KuduTable.isKuduTable(newTable)); : if (Table.isExternalTable(newTable)) { : KuduCatalogOpExecutor.populateExternalTableColsFromKudu(newTable); : } else { : KuduCatalogOpExecutor.createManagedTable(newTable, params); : } : boolean createsHMSTable; : if (Table.isExternalTable(newTable)) { : createsHMSTable = true; : } else { : String masterHosts = newTable.getParameters().get(KuduTable.KEY_MASTER_HOSTS); : String hmsUris; : // Check if Kudu's integration with the Hive Metastore is enabled for > Shouldn't be not be checking this in the case of an external table? Done http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@99 PS3, Line 99: . : if (KuduTable.isHMSIntegrationEnabled(masterHosts)) { > It's not clear where this is happening. Might be worth noting. Done http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@283 PS3, Line 283: // External table should not have table ID. > Not your fault, but it isn't quite obvious that this is an external table. Done http://gerrit.cloudera.org:8080/#/c/13375/3/tests/common/skip.py File tests/common/skip.py: http://gerrit.cloudera.org:8080/#/c/13375/3/tests/common/skip.py@110 PS3, Line 110: hms_integration > nit: maybe rename into hms_integration_enabled ? Done http://gerrit.cloudera.org:8080/#/c/13375/3/tests/common/skip.py@112 PS3, Line 112: not enabled.") > to not be set ? Done http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py@116 PS3, Line 116: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py@116 PS3, Line 116: > Could you add a brief description of this test, similar to L99? Done http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py@128 PS3, Line 128: @SkipIfKudu.no_hybrid_clock > What scenarios is this testing? Hard to tell without being more familiar wi Done. This only covers managed tables for now. I can add more tests for external tables once bump up the CDH_BUILD_NUMBER to include the latest Kudu bits as which includes the external tables handling. http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py@129 PS3, Line 129: def test_create_manged_kudu_tables(self, vector, unique_database): > flake8: W391 blank line at end of file Done -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Thu, 30 May 2019 04:49:54 +0000 Gerrit-HasComments: Yes
