Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21149 )
Change subject: IMPALA-12729: Allow creating primary keys for Iceberg tables ...................................................................... Patch Set 5: (15 comments) http://gerrit.cloudera.org:8080/#/c/21149/3/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/21149/3/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java@181 PS3, Line 181: if the column has options th > What are these incompatible with? Could you clarify the name or add a comme Done http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java File fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java: http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@180 PS3, Line 180: > Could this be an ELSE IF? Done http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@180 PS3, Line 180: isNullable_ = icebergColumn.isNullable(); > We could add a comment like on L174, or extend that one. Done http://gerrit.cloudera.org:8080/#/c/21149/3/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/21149/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@482 PS3, Line 482: > Nit: 'tables have' would be better, now that we modify this comment. Done http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@490 PS3, Line 490: if ( > Nit: ELSE IF should go on the previous line. Done http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@492 PS3, Line 492: if (isIcebergTable()) { > Why don't we need a similar clause for Iceberg? A comment could clarify. added a comment http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@492 PS3, Line 492: if (isIcebergTable()) { > Shouldn't we include this condition and return false in the "isIcebergTable no. This is the case when the table is not Iceberg, not Kudu but still some Kudu column options are provided. The reason we don't have to check for Iceberg options here is that Iceberg options are subset of Kudu options. http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@572 PS3, Line 572: } else if (!primaryKeyColNames_.isEmpty() && !isPrimaryKeyUnique() > If non-unique primary keys are now supported in Iceberg, shouldn't we modif L507 is a bit different. There are 2 ways to provide primary keys: 1: CREATE TABLE (i int not null primary key, j int); 2: CREATE TABLE (i int not null, j int, primary key(i)); With this patch we only support 2). That is more flexible btw, because you can add multiple cols into the PK, while with 1) you can't. Changed the comment at L89. http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@577 PS3, Line 577: > Are not enforced and non unique PKs the same? isPrimaryKeyUnique_ stores the info if NOT UNIQUE was provided or not. Added a comment about this at L89. http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java File fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java: http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java@174 PS3, Line 174: given columns > Could mention 'primaryKeyColumnNames'. Done http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java@192 PS3, Line 192: > Nit: Field. Done http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/21149/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2446 PS3, Line 2446: not enforced > Is this a breaking change if this test used to work without "not enforced"? well, previously Iceberg tables accepted primary key but it was simply ignored. I don't think this qualifies for a breaking change but let me know if you feel otherwise. Anyway this mimics the SQL syntax in Flink. http://gerrit.cloudera.org:8080/#/c/21149/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-create.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-create.test: http://gerrit.cloudera.org:8080/#/c/21149/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-create.test@619 PS3, Line 619: ---- QUERY > Could you add comments for the cases which don't have them? It would be inf Done http://gerrit.cloudera.org:8080/#/c/21149/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test: http://gerrit.cloudera.org:8080/#/c/21149/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test@887 PS3, Line 887: double > Could also add a similar test with double. Done http://gerrit.cloudera.org:8080/#/c/21149/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-negative.test@915 PS3, Line 915: > I think it could be interpreted as we can't delete 'i' because it's NOT NUL this error msg in fact comes from the Iceberg library. I found it verbose enough not to replace with any other messages. Additionally, it's not trivial once we caught an exception from Iceberg to decide what the root cause was without reading the actual message. -- To view, visit http://gerrit.cloudera.org:8080/21149 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7bea787acdabd8cb04661f4ddb5c3309af0364a6 Gerrit-Change-Number: 21149 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 17:01:32 +0000 Gerrit-HasComments: Yes
