Daniel Becker 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 5: (7 comments) Follow-up for my first partial review. http://gerrit.cloudera.org:8080/#/c/20760/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20760/5//COMMIT_MSG@26 PS5, Line 26: FlushFinal Can it spill to disk if the data it has received can't be stored in memory? http://gerrit.cloudera.org:8080/#/c/20760/5/be/src/exec/iceberg-buffered-delete-sink.h File be/src/exec/iceberg-buffered-delete-sink.h: http://gerrit.cloudera.org:8080/#/c/20760/5/be/src/exec/iceberg-buffered-delete-sink.h@72 PS5, Line 72: DCHECK(!file_pos.empty()); We could also DCHECK that none of the vectors in the map are empty either. http://gerrit.cloudera.org:8080/#/c/20760/5/be/src/exec/iceberg-buffered-delete-sink.h@91 PS5, Line 91: if (pos_level_it_ != file_level_it_->second.end()) { This condition should always be true, we've just dereferenced 'pos_level_it_' on L84 before calling this function. We should DCHECK it instead. http://gerrit.cloudera.org:8080/#/c/20760/3/be/src/exec/iceberg-buffered-delete-sink.h File be/src/exec/iceberg-buffered-delete-sink.h: http://gerrit.cloudera.org:8080/#/c/20760/3/be/src/exec/iceberg-buffered-delete-sink.h@45 PS3, Line 45: partition_descriptor_map_ Does 'partition_descriptor_map_' exist? http://gerrit.cloudera.org:8080/#/c/20760/3/be/src/exec/iceberg-buffered-delete-sink.h@48 PS3, Line 48: to the temporary Hdfs files > Sorry, this was copy-pasted, updated the comments. I can't see that it has changed in this file, though it did change in iceberg-delete-sink.h. Maybe those changes were intended here? Also, the other function descriptions that were copied here may contain legacy comments? http://gerrit.cloudera.org:8080/#/c/20760/3/be/src/exec/iceberg-buffered-delete-sink.h@69 PS3, Line 69: class FilePositionsIterator { > This way I can keep the typedefs private. The typedefs could stay here, the class could be forward-declared here and defined in the .cc file. http://gerrit.cloudera.org:8080/#/c/20760/5/be/src/exec/iceberg-delete-sink.h File be/src/exec/iceberg-delete-sink.h: http://gerrit.cloudera.org:8080/#/c/20760/5/be/src/exec/iceberg-delete-sink.h@66 PS5, Line 66: /// and the delete files as well. We could mention the case when the different deletes go to different sinks because of the differing new values and that we give an error in this case because we can't check it. -- 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: 5 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: Tue, 12 Dec 2023 10:34:01 +0000 Gerrit-HasComments: Yes
