Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19383 )
Change subject: WIP IMPALA-11809: Support non unique primary key for Kudu ...................................................................... Patch Set 9: (16 comments) Looks good! Will review the test part of the patch next. http://gerrit.cloudera.org:8080/#/c/19383/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19383/9//COMMIT_MSG@9 PS9, Line 9: Kudu engine adds support for non unique primary key. It adds nit recently http://gerrit.cloudera.org:8080/#/c/19383/9/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/19383/9/common/thrift/CatalogObjects.thrift@580 PS9, Line 580: add nit adds http://gerrit.cloudera.org:8080/#/c/19383/9/common/thrift/CatalogObjects.thrift@581 PS9, Line 581: . nit. ", in this case, this field is set to false" http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java: http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@99 PS9, Line 99: c.isPrimaryKeyUnique() ? "" : "non unique ", c.toString() This statement can be integrated into ColumnDef.toString(). http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@227 PS9, Line 227: createStmt_.isPrimaryKeyUnique(), createStmt_.getPrimaryKeyColumnDefs(), nit. It may be more logic to switch the two arguments, as the new argument further refines the primary key columns. http://gerrit.cloudera.org:8080/#/c/19383/9/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/19383/9/fe/src/main/java/org/apache/impala/analysis/TableDef.java@87 PS9, Line 87: An auto-incrementing column will be added : // automatically by Kudu engine as key column if primary key is not unique. nit. If not, an auto-incrementing column will be added automatically by Kudu engine. This extra key column helps produce a unique composite primary key (primary keys + auto-incrementing construct). http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/analysis/TableDef.java@537 PS9, Line 537: only the columns, on which the : // partitions are being created, nit. the partition columns have to be first in the primary key columns. http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/analysis/TableDef.java@566 PS9, Line 566: throw new AnalysisException(String.format("Multiple %sprimary keys specified. " + : "Composite %sprimary keys can be specified using the " + : "%sPRIMARY KEY (col1, col2, ...) syntax at the end of the column definition.", : isPrimaryKeyUnique() ? "" : "non unique ", : isPrimaryKeyUnique() ? "" : "non unique ", : isPrimaryKeyUnique() ? "" : "NON UNIQUE ")); nit. This looks similar to the lines starting at 520. http://gerrit.cloudera.org:8080/#/c/19383/9/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/19383/9/fe/src/main/java/org/apache/impala/catalog/Db.java@242 PS9, Line 242: boolean isPrimaryKeyUnique, List<ColumnDef> primaryKeyColumnDefs, As noted in another comment, these two arguments probably should be swapped. http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java File fe/src/main/java/org/apache/impala/catalog/KuduColumn.java: http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@134 PS9, Line 134: position nit. 'position'. http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@135 PS9, Line 135: if nit. when http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@144 PS9, Line 144: false One may call this function even when column.isIs_primary_key_unique() is true. Suggest to replace false with column.isIs_primary_key_unique(). http://gerrit.cloudera.org:8080/#/c/19383/9/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/19383/9/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@133 PS9, Line 133: if (isKey && !isKeyUnique) { : csb.nonUniqueKey(true); : } else { : csb.key(isKey); : } It seems the following covers all combos of isKey and isKeyUnique. csb.key(isKey); csb.nonUniqueKey(isKey && !isKeyUnique); http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@515 PS9, Line 515: String.format("Cannot add %s column to Kudu table %s", nit "as its name is identical to auto incremental column name in Kudu". http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@533 PS9, Line 533: String.format("Cannot drop %s column from Kudu table %s", nit. same comment as above. http://gerrit.cloudera.org:8080/#/c/19383/9/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@563 PS9, Line 563: String.format("Cannot alter %s column for Kudu table %s", nit same comment as above -- To view, visit http://gerrit.cloudera.org:8080/19383 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d7882bf3d01a3492cc9827c072d1f3200d9eebd Gerrit-Change-Number: 19383 Gerrit-PatchSet: 9 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Sun, 15 Jan 2023 23:44:40 +0000 Gerrit-HasComments: Yes
