Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20760 )
Change subject: IMPALA-12313: (part 3) Add UPDATE support for Iceberg tables. ...................................................................... Patch Set 12: (8 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20760/11/be/src/exec/iceberg-buffered-delete-sink.h File be/src/exec/iceberg-buffered-delete-sink.h: http://gerrit.cloudera.org:8080/#/c/20760/11/be/src/exec/iceberg-buffered-delete-sink.h@28 PS11, Line 28: > How come StringVal has to be redefined here? At some point I had to declare it, but you're right, now it is redundant. http://gerrit.cloudera.org:8080/#/c/20760/11/be/src/exec/iceberg-buffered-delete-sink.h@138 PS11, Line 138: > nit: empty space Done http://gerrit.cloudera.org:8080/#/c/20760/11/be/src/exec/iceberg-delete-sink-base.h File be/src/exec/iceberg-delete-sink-base.h: http://gerrit.cloudera.org:8080/#/c/20760/11/be/src/exec/iceberg-delete-sink-base.h@49 PS11, Line 49: Status ConstructPartitionInfo(const TupleRow* row, OutputPartition* output_partition); : Status ConstructPartitionInfo(int32_t spec_id, const std::string& partitions, : OutputPartition* output_partition); > nit: this should fit into one line Done http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java: http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@20 PS11, Line 20: import java.util.ArrayList; > unused import Done http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@30 PS11, Line 30: import org.apache.impala.common.AnalysisExc > unused import Done http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@37 PS11, Line 37: import org.apache.impala.thrift.TSortingOrder; > unused import Done http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/analysis/IcebergUpdateImpl.java@44 PS11, Line 44: // Id of the delete table in the descriptor t > unused import Done http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/planner/IcebergBufferedDeleteSink.java File fe/src/main/java/org/apache/impala/planner/IcebergBufferedDeleteSink.java: http://gerrit.cloudera.org:8080/#/c/20760/11/fe/src/main/java/org/apache/impala/planner/IcebergBufferedDeleteSink.java@34 PS11, Line 34: IcebergBufferedDeleteSink > For me it looks like that ~40% of this class is identical with IcebergDelet Yeah, actually I want to remove IcebergDeleteSink in the future: IMPALA-12640 Keeping them separate will make the removal easier. -- To view, visit http://gerrit.cloudera.org:8080/20760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bb97b4454165a292975d88dc9c23adb22ff7315 Gerrit-Change-Number: 20760 Gerrit-PatchSet: 12 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 15 Dec 2023 15:51:30 +0000 Gerrit-HasComments: Yes
