Zoltan Borok-Nagy 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 2: (28 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20078/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20078/1//COMMIT_MSG@60 PS1, Line 60: in a w > nit: "in a way" Done http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.h File be/src/exec/file-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.h@74 PS1, Line 74: > nit: comment or empty line Done 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 > For me 'Add' sounds better because it is similar to AddIcebergColumns. 'Set Yeah, I used 'Add' because we already have 'AddIcebergColumns'. Leave it as is for now, as there's no consensus on the new name. 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 poi Done http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@150 PS1, Line 150: DCHECK(slot_desc->IsVirtual()); > DCHECK that 'slot_desc' is indeed a virtual col? Done http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@178 PS1, Line 178: char* partition_serialized_copy = reinterpret_cast<char*>(mem_pool->Allocate(len)); > Can't we skip the copying part if the slot is null? Currently it's never NULL. It's either empty string or contain the serialized partition data. http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/file-metadata-utils.cc@185 PS1, Line 185: > nit: empty line Done http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/hdfs-table-sink.h File be/src/exec/hdfs-table-sink.h: http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/hdfs-table-sink.h@130 PS1, Line 130: usin > nit: outdated comment Done 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: /// Fills output_partition's partition_name, raw_partition_names and > comment pls Done http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.h@80 PS1, Line 80: > comment pls Done 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: ring& key) { > This function is called Get* but actually doesn't return anything. Simply t Renamed to 'SetCurrentPartition' http://gerrit.cloudera.org:8080/#/c/20078/1/be/src/exec/iceberg-delete-sink.cc@191 PS1, Line 191: #ifdef DEBUG > An empty else branch can be dropped. Done 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: Used > typo Done 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: fig& sink_c > Is it expected that sink_config.partition_key_exprs_ is the same as the 'pa Yes, good catch. http://gerrit.cloudera.org:8080/#/c/20078/1/common/thrift/DataSinks.thrift File common/thrift/DataSinks.thrift: http://gerrit.cloudera.org:8080/#/c/20078/1/common/thrift/DataSinks.thrift@107 PS1, Line 107: // IcebergDeleteSink. > nit: comment Done 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 contain the corresponding result expr > nit: I guess this is copy-paste but the line break seems off here. Done 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? Initially I only copied the members that were copied in the original classes (InsertStmt/ModifyStmt). The other members are set during analysis, so copying was not necessary I guess. Anyway, I added copying for consistency. http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java@93 PS1, Line 93: u > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/DmlStatementBase.java@108 PS1, Line 108: Preconditions.checkState(table_ instanceof FeKuduTable); > nit: could you move this next to the setter method? Done http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionSpec.java File fe/src/main/java/org/apache/impala/analysis/IcebergPartitionSpec.java: http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionSpec.java@61 PS1, Line 61: getSpe > nit: based on earlier getters, this should be getSpecId() Done 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@62 PS1, Line 62: lic abstract class ModifyStmt extends DmlSta > nit: outdated comment Done http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@328 PS1, Line 328: > nit: this could fit in one line. Done 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@180 PS1, Line 180: (int specI > nit: I think the parameter describes this part Done http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@182 PS1, Line 182: if (spec.getSpecId() == specId) return spec; > nit: body would fit in this line Done http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java File fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java: http://gerrit.cloudera.org:8080/#/c/20078/1/fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java@40 PS1, Line 40: Type.BIGINT, TVirtualColumnType.FILE_POSITION); > nit: new line Added comments instead 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 It was intentional, I think these method chains are more readable when they are aligned. 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 It was intentional, I think these method chains are more readable when they are aligned. 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: > For me it seems that the WHERE condition is only on int columns for these t Changed a few tests -- 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: 2 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 26 Jun 2023 17:25:33 +0000 Gerrit-HasComments: Yes
