Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/19776 )
Change subject: IMPALA-11877: (part 1) Add support for DELETE statements for UNPARTITIONED Iceberg tables ...................................................................... Patch Set 5: (11 comments) Thanks for this change, Zoltan! I spent some time reviewing the FE and thrift part of the change. Will continue with the rest. http://gerrit.cloudera.org:8080/#/c/19776/5/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/19776/5/common/thrift/CatalogObjects.thrift@81 PS5, Line 81: PARTITION_SPEC_ID, I haven't found any usage for these new virtual columns. I'd bet they are for the step2/delete from partitioned tables. Can we remove them from this patch then? http://gerrit.cloudera.org:8080/#/c/19776/5/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/19776/5/common/thrift/Query.thrift@827 PS5, Line 827: INSERT It's not just INSERT anymore, right? (same below) http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java File fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java: http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@162 PS5, Line 162: // Only Kudu tables can be updated This comment is off now. http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@172 PS5, Line 172: modifyImpl_ = this.new ModifyIceberg(); is 'this.' needed here? Looks bit weird to me. http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@174 PS5, Line 174: throw new AnalysisException( I believe this case is taken care by the check in L163. http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@362 PS5, Line 362: void setKuduTransactionToken(byte[] kuduTxnToken); This function (and the one below) seems implementation specific and seems odd to have Kudu related function in the parent class. Isn't there a way to remove these from the parent class? http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@466 PS5, Line 466: // TODO: rewrite DELETE FROM t; statements to TRUNCATE TABLE t; Could you please create a Jira for this and link it here? http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@482 PS5, Line 482: public ByteBuffer getKuduTransactionToken() { As written in an above comment, having Kudu specific functions in this Iceberg specific implementation seems odd. http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@300 PS5, Line 300: default boolean isPartitioned() { for my information: This returns true even if the table is not partitioned now but there was a partition spec in the past where it was partitioned? http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@304 PS5, Line 304: if (partField.getTransformType() == TIcebergPartitionTransformType.VOID) { nit: you can negate the condition here and return true instead of 'continue'. Might be slightly more readable. http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java File fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java: http://gerrit.cloudera.org:8080/#/c/19776/5/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java@42 PS5, Line 42: HdfsFileFormat.ICEBERG Technically Iceberg is not a file format. Would it break anything to remove it from this list? I did have another change the other day where I skipped adding HdfsFileFormat.ICEBERG into the file formats of a table. That might help removing this. -- To view, visit http://gerrit.cloudera.org:8080/19776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic933b2295abe54b46d2a736961219988ff42915b Gerrit-Change-Number: 19776 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 10 May 2023 08:46:40 +0000 Gerrit-HasComments: Yes
