Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21932 )
Change subject: IMPALA-13509: Copy rows directly to OutboundRowBatch during hash partitioning ...................................................................... Patch Set 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/21932/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21932/9//COMMIT_MSG@21 PS9, Line 21: PartitionRowCollector's OutboundRowBatche (collector_batch_), which typo: OutboundRowBatch http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender-ir.cc File be/src/runtime/krpc-data-stream-sender-ir.cc: http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender-ir.cc@55 PS9, Line 55: num_rows_ ++; nit: we don't usually have a space between id and '++'. http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender.cc@194 PS9, Line 194: // Copies a single row into this channel's row batch and flushes the row batch once Is this comment still accurate? http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender.cc@602 PS9, Line 602: COUNTER_ADD(parent_->outbound_row_batches_sent_counter_, 1); Does this count successful sends, or attempts to send (some of which may fail)? I would have assumed successful batches sent, which we'd want to increment after DoTransmitDataRpc succeeds. http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender.cc@1240 PS9, Line 1240: { I don't see a reason to add this additional block. They're already in an else block that ends just after it. http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender.cc@1351 PS9, Line 1351: compress ? compression_scratch_.get() : nullptr, true)); nit: think this should be indented 4 more spaces. http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/krpc-data-stream-sender.cc@1353 PS9, Line 1353: COUNTER_ADD(uncompressed_bytes_counter_, uncompressed_bytes); Why don't num_receivers factor in here? http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/outbound-row-batch.h File be/src/runtime/outbound-row-batch.h: http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/outbound-row-batch.h@77 PS9, Line 77: Status PrepareForSend(int num_tuples_per_row, TrackedString* compression_scratch, Maybe default used_append_row=false to avoid impacting existing users? Would make any cherry-picks simpler. http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/outbound-row-batch.inline.h File be/src/runtime/outbound-row-batch.inline.h: http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/outbound-row-batch.inline.h@53 PS9, Line 53: // resizing to the exact size, similarly to vector. It be would clearer to use a typo: It would be clearer http://gerrit.cloudera.org:8080/#/c/21932/9/be/src/runtime/outbound-row-batch.inline.h@75 PS9, Line 75: DCHECK(&tuple_data_[0] != nullptr); > clang tidy is complaining about this: I would agree, it seems to be checking something that couldn't be null. You could test tuple_data.data(), but not sure why you think that would ever be null. -- To view, visit http://gerrit.cloudera.org:8080/21932 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I81a16c2f0fcfc1f3adef7077b3932a29a0f15a8f Gerrit-Change-Number: 21932 Gerrit-PatchSet: 9 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 05 Nov 2024 00:09:25 +0000 Gerrit-HasComments: Yes
