Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19383 )
Change subject: IMPALA-11809: Support non unique primary key for Kudu ...................................................................... Patch Set 17: (7 comments) http://gerrit.cloudera.org:8080/#/c/19383/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19383/16//COMMIT_MSG@17 PS16, Line 17: The assignment to it during insertion is automatic so : insertion statements should not specify values for auto-incrementing : column. > Is there any test that explicitly trying to insert into auto_incrementing_i Kudu server will return error if auto_incrementing_id is set. Added some negative test cases in kudu_insert.test. http://gerrit.cloudera.org:8080/#/c/19383/16/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: http://gerrit.cloudera.org:8080/#/c/19383/16/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java@342 PS16, Line 342: sb.append(" ").append(KuduUtil.getPrimaryKeyString(isPrimaryKeyUnique_)); : } > nit: Use KuduUtil.getPrimaryKeyString() here? Done http://gerrit.cloudera.org:8080/#/c/19383/16/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/19383/16/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@488 PS16, Line 488: sb.append(KuduUtil.getPrimaryKeyString(isPrimaryKeyUnique)).append(" ("); : Joiner.on(", ").appendTo(sb > nit: Use KuduUtil.getPrimaryKeyString() here? Done http://gerrit.cloudera.org:8080/#/c/19383/16/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@501 PS16, Line 501: Joiner.on(", ").appendTo(sb, primaryKeysSql).append(")"); : } > nit: Use KuduUtil.getPrimaryKeyString() here? Done http://gerrit.cloudera.org:8080/#/c/19383/16/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/16/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@72 PS16, Line 72: || (isKey && (isPrimaryKeyUnique && !isAutoIncrementing || !isPrimaryKeyUnique))); : kuduName_ = name; : isKey_ = isKey; : isPrimaryKeyUnique_ = isPrimaryKeyUnique; > Does it make sense to add Preconditions here to verify valid combo between Done http://gerrit.cloudera.org:8080/#/c/19383/16/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/19383/16/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@391 PS16, Line 391: isPrimaryKeyUnique_ = kuduSchema_.isPrimaryKeyUnique(); : hasAutoIncrementingColumn_ = kuduSchema_.hasAutoIncrementingColumn(); > If isPrimaryKeyUnique() return False, does hasAutoIncrementingColumn() guar No. auto-incrementing column must be non unique primary key, but non unique primary key may not be auto-incrementing column. http://gerrit.cloudera.org:8080/#/c/19383/16/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/19383/16/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@690 PS16, Line 690: AnalysisException: Column already exists: auto_incrementing_id > If auto_incrementing_id is reserved, is it valid to create / have existing No, Kudu return error. There is a test case which fails to add auto_incrementing_id column with ALTER TABLE. Added a test case which fails to create a table with auto_incrementing_id column. -- 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: 17 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: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Fri, 03 Feb 2023 08:05:19 +0000 Gerrit-HasComments: Yes
