Riza Suminto 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: (4 comments) 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 > Done 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(")"); : } > Done Done http://gerrit.cloudera.org:8080/#/c/19383/17/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/17/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@71 PS17, Line 71: Preconditions.checkArgument((!isKey && !isPrimaryKeyUnique && !isAutoIncrementing) : || (isKey && (isPrimaryKeyUnique && !isAutoIncrementing || !isPrimaryKeyUnique))); May I suggest to split this with branch on isKey? if (isKey) { Preconditions.checkArgument(isPrimaryKeyUnique && !isAutoIncrementing || !isPrimaryKeyUnique); } else { Preconditions.checkArgument(!isPrimaryKeyUnique && !isAutoIncrementing); } This way, we can quickly distinguish between the two case on failure. 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(); > No. auto-incrementing column must be non unique primary key, but non unique So is it valid to add this precondition here? Preconditions.checkState(!isPrimaryKeyUnique_ || !hasAutoIncrementingColumn_); -- 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 16:52:10 +0000 Gerrit-HasComments: Yes
