Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21435 )
Change subject: IMPALA-13088: (part 1) Improve build batch processing of IcebergDeleteBuilder ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.h File be/src/exec/iceberg-delete-builder.h: http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.h@95 PS2, Line 95: // Checks distribution mode and collects the processed data files' file path in case The function comment is not valid now. Would be nice to mention that this populates the keys of intermediate_delete_rows_ and adds empty vectors to the values. http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.h@124 PS2, Line 124: using IntermediateDeleteRowHashTable = Can you add a comment what the keys and values are for in this map? http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.h@146 PS2, Line 146: IntermediateDeleteRowVector& : intermediate_vector nit: seems a weird place to break the line http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.cc File be/src/exec/iceberg-delete-builder.cc: http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.cc@184 PS2, Line 184: retval.first->second. To increase readability, do you think it'd make sense to introduce a variable/reference for 'retval.first->second'? http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.cc@259 PS2, Line 259: std::unique nit: I'd break the line before the first param, and then each param could fit into a (separate) single line. http://gerrit.cloudera.org:8080/#/c/21435/2/be/src/exec/iceberg-delete-builder.cc@325 PS2, Line 325: ErrorMsg(TErrorCode::GENERAL, "NULL found as file_path in delete file")); KrpcDataStreamSender already filters out NULL file_paths. Is there a need to add an error message here too? Can't we DCHECK for non-nullness? Or is this because of some data corruption scenario? -- To view, visit http://gerrit.cloudera.org:8080/21435 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14541a064a522d4780fb5f02636736259e79b9cf Gerrit-Change-Number: 21435 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 30 May 2024 14:58:39 +0000 Gerrit-HasComments: Yes
