Csaba Ringhofer 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: (7 comments) http://gerrit.cloudera.org:8080/#/c/24089/21//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24089/21//COMMIT_MSG@26 PS21, Line 26: Measurements: Similarly to https://gerrit.cloudera.org/#/c/24132/ , it would be nice how some number like number of instructions generated http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/CMakeLists.txt File be/src/runtime/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/CMakeLists.txt@99 PS21, Line 99: variant-value.cc nit: it is often better to do the rebase and actual changes in separate patchets http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream-ir.cc File be/src/runtime/buffered-tuple-stream-ir.cc: http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream-ir.cc@47 PS21, Line 47: check_null nit: indicate in the name that it is about tuple nullnes? 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); > I don't understand what you mean. ack, discussed offline, I didn't spot the ++row_idx part http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream-test.cc@2391 PS21, Line 2391: CreateDescriptors Why do the codegen tests use a different tuple layout? Ideally the same tests could be run with interpreted and codegened code. http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream.cc@1078 PS21, Line 1078: codegen->true_value() : codegen->false_value(); nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream.cc@1254 PS21, Line 1254: for (int i = 0; i < tuples_per_row; ++i) { : const int tuple_size = fixed_tuple_sizes_[i]; : if (UNLIKELY(!CopyTupleFixedLen(row, i, tuple_size, &pos, data_end, true))) { : return false; : } : } : } else { : // If we know that there are no nullable tuples no need to set the nullability flags. : for (int i = 0; i < tuples_per_row; ++i) { : const int tuple_size = fixed_tuple_sizes_[i]; : // TODO: Once IMPALA-1306 (Avoid passing empty tuples of non-materialized slots) : // is delivered, the check below should become DCHECK(row->GetTuple(i) != nullptr). : DCHECK(row->GetTuple(i) != nullptr || tuple_size == 0); : if (UNLIKELY(!CopyTupleFixedLen(row, i, tuple_size, &pos, data_end, false))) { : return false; : } The two blocks could merged by passing HAS_NULLABLE_TUPLE as last parameter of CopyTupleFixedLen. -- 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: Wed, 01 Jul 2026 12:14:05 +0000 Gerrit-HasComments: Yes
