Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24154 )
Change subject: IMPALA-14852 Codegen tuple TryDeepCopy for Broadcast Exchange ...................................................................... Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/24154/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24154/11//COMMIT_MSG@16 PS11, Line 16: Can you mention that collections are still handled in an interpreted way? http://gerrit.cloudera.org:8080/#/c/24154/11//COMMIT_MSG@27 PS11, Line 27: SerializeBatchTime changed from 1s797ms to 1s239ms. Can you check if there is change if codegen is turned off? http://gerrit.cloudera.org:8080/#/c/24154/11//COMMIT_MSG@28 PS11, Line 28: This is about 45% improvement. shouldn't we calculate this compared to the larger value? http://gerrit.cloudera.org:8080/#/c/24154/11/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/24154/11/be/src/runtime/krpc-data-stream-sender-ir.cc@74 PS11, Line 74: RETURN_IF_ERROR( : src->Serialize(dest, compress ? compression_scratch_.get() : nullptr)); nit: I would prefer to have only an inner function in ir.cc instead of the whole things - IMO things like the timer and calling compression shouldn't be here http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch-ir.cc File be/src/runtime/outbound-row-batch-ir.cc: http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch-ir.cc@80 PS11, Line 80: bool IR_ALWAYS_INLINE StatusOK(Status* status) { : return status->ok(); : } Is this used somewhere? http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.h File be/src/runtime/outbound-row-batch.h: http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.h@81 PS11, Line 81: assumetyps typo http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.h@94 PS11, Line 94: class DedupMap : public FixedSizeHashTable<Tuple*, int> { : public: : static const char* LLVM_CLASS_NAME; : }; : : Status IR_ALWAYS_INLINE AppendTupleWithDedup(const TupleRow* row, : const TupleRow* prev_row, int tuple_idx, DedupMap* distinct_tuples, : TupleDescriptor* desc, int byte_size, int num_tuples); : : Status IR_NO_INLINE AppendRowWithDedup( : const TupleRow* row, const TupleRow* prev_row, DedupMap* distinct_tuples, : const RowDescriptor* row_desc) noexcept; Adding some comments related to deduplication or pointing to another place that explains it would be nice. http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.cc File be/src/runtime/outbound-row-batch.cc: http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.cc@146 PS11, Line 146: llvm::Value* tuple_desc It looks unusual that we get tuple desc as value. This is needed to be able to deepcopy collections, right, as that part is not fully codegend? Can you add a comment about this? Btw would it make sense to not pass this if there are no collections? http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.inline.h File be/src/runtime/outbound-row-batch.inline.h: http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/outbound-row-batch.inline.h@25 PS11, Line 25: #include "util/fixed-size-hash-table.h" Where is this used? http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/tuple.cc@594 PS11, Line 594: Value here and at a few other other places using llvm::Constant* seems clearer to me It may be even better to add constness somehow to the variable name to make compile vs runtime dataflow easier to follow. http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/tuple.cc@599 PS11, Line 599: succeed nit: indicating that this means "data_end was reached" would be clearer http://gerrit.cloudera.org:8080/#/c/24154/11/be/src/runtime/tuple.cc@641 PS11, Line 641: builder.CreateStore(data_start, data); : builder.CreateStore(offset_start, offset); Is resetting needing? The non-codegen code doesn't seem to do this. -- To view, visit http://gerrit.cloudera.org:8080/24154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa43e949c63db047717104dbbe42d47f94ebf2d0 Gerrit-Change-Number: 24154 Gerrit-PatchSet: 11 Gerrit-Owner: Balazs Hevele <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 29 Jun 2026 17:13:28 +0000 Gerrit-HasComments: Yes
