Balazs Hevele has posted comments on this change. ( http://gerrit.cloudera.org:8080/24154 )
Change subject: IMPALA-14852 Codegen tuple TryDeepCopy for Broadcast Exchange ...................................................................... Patch Set 12: (12 comments) Thanks for the review! 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? Done http://gerrit.cloudera.org:8080/#/c/24154/11//COMMIT_MSG@27 PS11, Line 27: bin/load-data.py -s 30 -f --workloads tpch > Can you check if there is change if codegen is turned off? With codegen turned off, it is about the same as before the change, for this particular query. http://gerrit.cloudera.org:8080/#/c/24154/11//COMMIT_MSG@28 PS11, Line 28: --table_formats text/none > shouldn't we calculate this compared to the larger value? Done 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: : > nit: I would prefer to have only an inner function in ir.cc instead of the Done 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? Yes, this is used in OutboundRowBatch::CodegenAppendRowWithDedup to early return if AppendTuple failed for a tuple in the row. 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: assumes th > typo Done 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; : }; : : // Append tuple/row with deduplication: : // -nullptr tuples will be encoded as -1 in tuple_offsets_ : // -as a fast deduplication for adjacent rows, if the tuple points to the same memory : // as the previous row's corresponding tuple, its offset will be duplicated in : // tuple_offsets_, and the call to AppendTuple will be spared : // -optionally, if a DedupMap is provided in distinct_tuples, the tuple's hash will be : // compared against all previous tuples, an > Adding some comments related to deduplication or pointing to another place Done 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: // This is passed throu > It looks unusual that we get tuple desc as value. This is needed to be able I added a comment clarifying this. Currently, there is no easy way to have a separate function signature without a TupleDesc* argument as long as collection types are not codegen'd. Added a todo for that. 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: > Where is this used? Removed it. 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: Const > here and at a few other other places using llvm::Constant* seems clearer to Done 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 Done 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. The interpreted code does it the other way: -creates a copy of the pointer -it advances the value of the copy -only updates the passed in pointer if the deepcopy succeeded The comment in tuple.h also explicitly says "If it fails, 'data' will be the same as before the call", though I didn't check if any uses rely on that. It was easier to reset the passed in argument upon failure in codegen code, but it should be possible to do it the other way as well. -- 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: 12 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, 20 Jul 2026 08:48:02 +0000 Gerrit-HasComments: Yes
