Wenzhe Zhou 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) Thanks Qifan for your review. 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 Done 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 Done 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" Done 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(). use KuduUtil.getPrimaryKeyString() 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 Done 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 Done 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. Done 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. use KuduUtil.getPrimaryKeyString() 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 Done 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'. Done 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 Done 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 tr This is a static function and it is only called when the primary key of the table is not 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. Kudu client API ColumnSchemaBuilder.key() and ColumnSchemaBuilder.nonUniqueKey() overwrite the previous calls. ColumnSchemaBuilder.nonUniqueKey(false) set the the column as non key, i.e both key and keyUnique as false. 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". Remove lines #512 to #518 since Kudu client API AlterTableOptions.addColumn() checks if the column name is identical to its reserved column name. 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. Remove lines #530 to #535 since Kudu client API AlterTableOptions.dropColumn() checks if the column name is identical to its reserved column name. 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 Remove lines #560 to #565 since Kudu client AlterTableOptions APIs checks if the column name is identical to its reserved column name. -- 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: Tue, 17 Jan 2023 06:02:27 +0000 Gerrit-HasComments: Yes
