Baike Xia has posted comments on this change. ( http://gerrit.cloudera.org:8080/19055 )
Change subject: IMPALA-3119: DDL support for bucketed tables ...................................................................... Patch Set 17: (12 comments) Hi Quanlong, thanks for your review and comments. I have fixed your comments. When testing 'show-create-table', I found a bug, and fixed it, and added support for bucketed table deletion. http://gerrit.cloudera.org:8080/#/c/19055/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19055/16//COMMIT_MSG@21 PS16, Line 21: he hash functi > nit: "the hash function used in Hive's bucketed tables" Done http://gerrit.cloudera.org:8080/#/c/19055/16//COMMIT_MSG@22 PS16, Line 22: > nit: currently don't Done http://gerrit.cloudera.org:8080/#/c/19055/16/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/19055/16/common/thrift/CatalogObjects.thrift@155 PS16, Line 155: tion > nit: "type" ? Maybe that makes it easier to understand: 'Data distribution method of bucketed table.' http://gerrit.cloudera.org:8080/#/c/19055/16/common/thrift/CatalogObjects.thrift@194 PS16, Line 194: 2: optional i64 total_file_bytes : } > nit: The variable names are clear enough. We can simplify the comment to so Done http://gerrit.cloudera.org:8080/#/c/19055/16/common/thrift/CatalogObjects.thrift@497 PS16, Line 497: optional TValidWriteIdList > nit: "Bucket information for HDFS tables" Done http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/analysis/TableDef.java@404 PS16, Line 404: isBucketableFormat() { > nit: it'd be better to rename it to something like "isBucketableFormat" Great. http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/analysis/TableDef.java@756 PS16, Line 756: yzeBucketColumns(options_.bucketInfo, getColumnNames(), > nit: we can skip this check since it's done in the following method. Done http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/analysis/TableDef.java@778 PS16, Line 778: "'%s'", options_.fileFormat)); : } : if (bucketInfo.getNum_bucket() <= 0) { : > nit: kudu is checked in isSupportBucketedTable(). Do we still need this che Done http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/util/BucketUtils.java File fe/src/main/java/org/apache/impala/util/BucketUtils.java: http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/util/BucketUtils.java@20 PS16, Line 20: import org.apache.hadoop.hive.metastore.api.StorageDescriptor; > nit: unused import Done http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/util/BucketUtils.java@31 PS16, Line 31: mStorageDescriptor(StorageDe > nit: "StorageDescriptor of the HMS table" Done http://gerrit.cloudera.org:8080/#/c/19055/16/testdata/workloads/functional-query/queries/QueryTest/create-table.test File testdata/workloads/functional-query/queries/QueryTest/create-table.test: http://gerrit.cloudera.org:8080/#/c/19055/16/testdata/workloads/functional-query/queries/QueryTest/create-table.test@349 PS16, Line 349: ---- RESULTS: VERIFY_IS_SUBSET > Can we add the rows of "Num Buckets" and "Bucket Columns" ? Done http://gerrit.cloudera.org:8080/#/c/19055/16/testdata/workloads/functional-query/queries/QueryTest/show-create-table.test File testdata/workloads/functional-query/queries/QueryTest/show-create-table.test: http://gerrit.cloudera.org:8080/#/c/19055/16/testdata/workloads/functional-query/queries/QueryTest/show-create-table.test@1013 PS16, Line 1013: 'engine.hive.enabled'='true', 'table_type'='ICEBERG', 'write.merge.mode'='copy-on-write') > Could you also add a test for bucket table in this file? Done -- To view, visit http://gerrit.cloudera.org:8080/19055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I919b4d4139bc3a7784fa6fdb6f064e25666d548e Gerrit-Change-Number: 19055 Gerrit-PatchSet: 17 Gerrit-Owner: Baike Xia <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Baike Xia <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 02 Nov 2022 11:35:27 +0000 Gerrit-HasComments: Yes
