Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/20078 )
Change subject: IMPALA-11877: (part 2) Add support for DELETE statements for PARTITIONED Iceberg tables ...................................................................... Patch Set 1: (18 comments) Thanks for the change, Zoli! One extra feedback: for reviews of this size I think it could help if the refactor part went into a separate release as I spent decent amount of time tracking if a change is for the new feature or for the refactor. Otherwise I haven't found any serious issues, mostly nits. http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc File be/src/exec/file-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@147 PS1, Line 147: AddVirtualIcebergColumn I think this is not an 'Add' method but rather a 'Fill' method. FillVirtualIcebergColumns? http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@148 PS1, Line 148: ice_metadata 'ice_metadata' is an IN param. shouldn't it be a const ref instead of a pointer? http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@150 PS1, Line 150: if (slot_desc->virtual_column_type() == TVirtualColumnType::PARTITION_SPEC_ID) { DCHECK that 'slot_desc' is indeed a virtual col? http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@178 PS1, Line 178: Ubsan::MemCpy(partition_serialized_copy, partitions.c_str(), len); Can't we skip the copying part if the slot is null? http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.h File be/src/exec/iceberg-delete-sink.h: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.h@72 PS1, Line 72: void ConstructPartitionInfo( comment pls http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.h@80 PS1, Line 80: Status GetOutputPartition(RuntimeState* state, const TupleRow* row, comment pls http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.cc File be/src/exec/iceberg-delete-sink.cc: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.cc@114 PS1, Line 114: GetOutputPartition This function is called Get* but actually doesn't return anything. Simply taking a look at the name doesn't really tells us what this does. http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.cc@191 PS1, Line 191: } else { An empty else branch can be dropped. http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/output-partition.h File be/src/exec/output-partition.h: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/output-partition.h@70 PS1, Line 70: Uset typo http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/table-sink-base.h File be/src/exec/table-sink-base.h: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/table-sink-base.h@40 PS1, Line 40: sink_config Is it expected that sink_config.partition_key_exprs_ is the same as the 'partition_key_exprs' param of this constructor? If yes then we can remove the 'partition_key_exprs' config, can't we? http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java File fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java: http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java@37 PS1, Line 37: // 'sort.columns' table property, this list will nit: I guess this is copy-paste but the line break seems off here. http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java@67 PS1, Line 67: protected DmlStatementBase(DmlStatementBase other) { Shouldn't this copy the other class members as well? http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java@108 PS1, Line 108: public java.nio.ByteBuffer getKuduTransactionToken() { nit: could you move this next to the setter method? http://gerrit.cloudera.org:8080/#/c/20078/1/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/20078/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@328 PS1, Line 328: if (isSortingColumn) { nit: this could fit in one line. http://gerrit.cloudera.org:8080/#/c/20078/1/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/20078/1/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@182 PS1, Line 182: if (spec.specId() == specId) { nit: body would fit in this line http://gerrit.cloudera.org:8080/#/c/20078/1/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/20078/1/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java@102 PS1, Line 102: .append(Expr.getExplainString(outputExprs_, explainLevel) + "\n"); nit: I think the indentation is off here http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java@105 PS1, Line 105: .append(Expr.getExplainString(partitionKeyExprs_, explainLevel) + "\n"); nit: indentation http://gerrit.cloudera.org:8080/#/c/20078/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-partitioned.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-partitioned.test: http://gerrit.cloudera.org:8080/#/c/20078/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-delete-partitioned.test@344 PS1, Line 344: WHERE i % 10 = 0 For me it seems that the WHERE condition is only on int columns for these tests. Would you mind testing it with other types and also functions like "length(string_col) < 3"? -- To view, visit http://gerrit.cloudera.org:8080/20078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28b06f240c23c336a7c5b6ef22fe2ee0a21f7b60 Gerrit-Change-Number: 20078 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Comment-Date: Thu, 22 Jun 2023 12:53:51 +0000 Gerrit-HasComments: Yes
