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 16: (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_id column? Say, INSERT INTO tbl_name (auto_incrementing_id) VALUES (0); 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: if (!isPrimaryKeyUnique_) sb.append(" NON UNIQUE"); : sb.append(" PRIMARY KEY"); nit: Use KuduUtil.getPrimaryKeyString() here? 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: if (!isPrimaryKeyUnique) sb.append("NON UNIQUE "); : sb.append("PRIMARY KEY ("); nit: Use KuduUtil.getPrimaryKeyString() here? http://gerrit.cloudera.org:8080/#/c/19383/16/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@501 PS16, Line 501: if (!isPrimaryKeyUnique) sb.append("NON UNIQUE "); : sb.append("PRIMARY KEY ("); nit: Use KuduUtil.getPrimaryKeyString() here? 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_ = isKey; : isPrimaryKeyUnique_ = isPrimaryKeyUnique; : isNullable_ = isNullable; : isAutoIncrementing_ = isAutoIncrementing; Does it make sense to add Preconditions here to verify valid combo between these 4 attributes? 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() guaranteed by Kudu client to return True? 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 Kudu table with explicit column name auto_incrementing_id even if there is no NON UNIQUE PRIMARY KEY? -- 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: 16 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 03 Feb 2023 00:58:32 +0000 Gerrit-HasComments: Yes
