Balazs Hevele has posted comments on this change. ( http://gerrit.cloudera.org:8080/24089 )
Change subject: IMPALA-14850: Codegen tuple DeepCopy for hash join ...................................................................... Patch Set 21: (10 comments) http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream-ir.cc File be/src/runtime/buffered-tuple-stream-ir.cc: http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream-ir.cc@83 PS20, Line 83: ections(tuple, i > Does this function have to be in .ir.cc? Is this ever called from codegened Done http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream-ir.cc@95 PS20, Line 95: template bool BufferedTupleStream::DeepCopyCollections<false> : (TupleRow* row, uint8_t** data, const uint8_t* data_end) noexcept; : : : : : : : : : : : : : : : : : : : > Wouldn't it make sense to reuse CopyTupleFixedLen here to keep codegened an Done http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream-test.cc@198 PS20, Line 198: GenBoolValue(idx); > Maybe do this in less cases to have null slots even in the the case when th I don't understand what you mean. Currently the tuple nullness and slot nullness are independent: if both gen_null_tuples and gen_null_slots are true, we can have null tuples, non-null tuples with null slots and non-null tuples with non-null slots. http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.h@731 PS20, Line 731: uint8_ > This is int8_t, while other functions use uint8_t - is there a reason for t Done http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1001 PS20, Line 1001: tuple > This copies a whole row, not just a single tuple. Updated the comment. http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1072 PS20, Line 1072: : // Copy fixed sized tuple data : for (int i = 0; i < desc->tuple_descriptors().size(); ++i) { : // bool result = CopyTupleFixedLen(row, tuple_idx, tuple_size, data, data_end); : > fixed_tuple_size_total is computed but not used Done http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1087 PS20, Line 1087: ontext, "conti > nullable_tuple suggests to me the this is checked per tuple, not for the wh Thanks for catching that, fixed http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1127 PS20, Line 1127: llvm::Value* tuple_offset = codegen->GetI32Constant(slot_desc->tuple_offset()); > nit: +2 indentation Done http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1129 PS20, Line 1129: llvm::Value* result = codegen->CodegenCallFunction(&builder, > nit: +2 indentation Done http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1191 PS20, Line 1191: } > After the codegen->FinalizeFunction() calls I checked we don't add a dcheck Done -- To view, visit http://gerrit.cloudera.org:8080/24089 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e32babdbaf56095478c6c66afb9cb91189f946 Gerrit-Change-Number: 24089 Gerrit-PatchSet: 21 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 30 Jun 2026 18:05:19 +0000 Gerrit-HasComments: Yes
