Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21452 )
Change subject: IMPALA-13088: (part 2) Parallelize final sorts in IcebergDeleteBuilder ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/21452/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21452/2//COMMIT_MSG@33 PS2, Line 33: build fragments aren't these 'probe' fragments that are blocked on the builder? http://gerrit.cloudera.org:8080/#/c/21452/2//COMMIT_MSG@37 PS2, Line 37: The nit: typo http://gerrit.cloudera.org:8080/#/c/21452/2//COMMIT_MSG@57 PS2, Line 57: lowered reduced? http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/iceberg-delete-builder.h File be/src/exec/iceberg-delete-builder.h: http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/iceberg-delete-builder.h@147 PS2, Line 147: Status FinalizeBuildImplParallel(int num_threads); Since this one can return error codes too, could you add a comment in what cases can we except errors? http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/iceberg-delete-builder.cc File be/src/exec/iceberg-delete-builder.cc: http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/iceberg-delete-builder.cc@314 PS2, Line 314: IcebergDeleteBuilder::DeleteRowVector IcebergDeleteBuilder::GetFinalDeleteRowVector( This and the other PR made me think about whther it makes sense to create a separate class for this DeleteRowVector where we can encapsulate all the logic that is needed to populate the vector of vectors, allocating new vectors with increasing size, and also to flatten the vector of vectors into one vector. It would reduce code complexity from the builder code and would give the responsibility of representing the positions in a more efficient way to another code-level. What do you think? http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/iceberg-delete-builder.cc@320 PS2, Line 320: int64_t capacity = 0; indentation http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/join-builder.h File be/src/exec/join-builder.h: http://gerrit.cloudera.org:8080/#/c/21452/2/be/src/exec/join-builder.h@197 PS2, Line 197: int probes_waiting_on_builder_ = 0; The variable name is self-explanatory, but some additional comment that is a bit more verbose could be useful. -- To view, visit http://gerrit.cloudera.org:8080/21452 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ca946a452d061238255e9b0e2c81a51cac68807 Gerrit-Change-Number: 21452 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: Fri, 31 May 2024 08:51:07 +0000 Gerrit-HasComments: Yes
