Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19055 )
Change subject: IMPALA-3119: DDL support for bucketed tables ...................................................................... Patch Set 16: Code-Review+1 (12 comments) Thanks for updating the syntax! I only have some minor comments. 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: a hash of Hive nit: "the hash function used in Hive's bucketed tables" http://gerrit.cloudera.org:8080/#/c/19055/16//COMMIT_MSG@22 PS16, Line 22: that do not nit: currently don't 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: table nit: "type" ? http://gerrit.cloudera.org:8080/#/c/19055/16/common/thrift/CatalogObjects.thrift@194 PS16, Line 194: // When create bucketd table, need to know : // about bucket's type, bucket's columns and number. nit: The variable names are clear enough. We can simplify the comment to something like "Represents the bucket spec of a table". http://gerrit.cloudera.org:8080/#/c/19055/16/common/thrift/CatalogObjects.thrift@497 PS16, Line 497: Bucket type, columns, number nit: "Bucket information for HDFS tables" 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: isSupportBucketedTable nit: it'd be better to rename it to something like "isBucketableFormat" http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/analysis/TableDef.java@756 PS16, Line 756: && options_.bucketInfo.getBucket_type() != TBucketType.NONE nit: we can skip this check since it's done in the following method. http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/analysis/TableDef.java@778 PS16, Line 778: if (isKuduTable()) { : throw new AnalysisException(String.format("CLUSTERED BY not supported for Kudu " + : "tables.")); : } nit: kudu is checked in isSupportBucketedTable(). Do we still need this check? 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 java.util.Map; nit: unused import http://gerrit.cloudera.org:8080/#/c/19055/16/fe/src/main/java/org/apache/impala/util/BucketUtils.java@31 PS16, Line 31: hive table'StorageDescriptor nit: "StorageDescriptor of the HMS table" 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" ? 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? -- 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: 16 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: Tue, 01 Nov 2022 13:33:01 +0000 Gerrit-HasComments: Yes
