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 20: (10 comments) Gave it another pass, sorry for the long delay. The tests were review only at a superficial level. 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: DeepCopyInternal Does this function have to be in .ir.cc? Is this ever called from codegened code? http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream-ir.cc@95 PS20, Line 95: for (int i = 0; i < tuples_per_row; ++i) { : const int tuple_size = fixed_tuple_sizes_[i]; : Tuple* t = row->GetTuple(i); : if (t != nullptr) { : if (UNLIKELY(pos + tuple_size > data_end)) return false; : memcpy(pos, t, tuple_size); : pos += tuple_size; : } : } : } 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]; : if (UNLIKELY(pos + tuple_size > data_end)) return false; : Tuple* t = row->GetTuple(i); : // TODO: Once IMPALA-1306 (Avoid passing empty tuples of non-materialized slots) : // is delivered, the check below should become DCHECK(t != nullptr). : DCHECK(t != nullptr || tuple_size == 0); : memcpy(pos, t, tuple_size); : pos += tuple_size; : } Wouldn't it make sense to reuse CopyTupleFixedLen here to keep codegened and interpreted code closer? 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 there are null tuples? E.g. every second case where the slow would be null thew whole tuple could be null 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: int8_t This is int8_t, while other functions use uint8_t - is there a reason for the difference? 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. http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1072 PS20, Line 1072: // Copy fixed sized tuple data : int fixed_tuple_size_total = 0; : for (const TupleDescriptor* tuple_desc : desc->tuple_descriptors()) { : fixed_tuple_size_total += tuple_desc->byte_size(); : } fixed_tuple_size_total is computed but not used http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1087 PS20, Line 1087: nullable_tuple nullable_tuple suggests to me the this is checked per tuple, not for the whole row, but line 1052 sets it for the whole. It seems easy to set it per tuple. fyi nullable tuple comes for outer joins - in a chain of join where the row has many tuples at the end it is not unusual to have both nullable and non-nullable tuples, e.g. if some joins are left joins, some are inner joins http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1127 PS20, Line 1127: slot_desc->null_indicator_offset().byte_offset); nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1129 PS20, Line 1129: slot_desc->null_indicator_offset().bit_mask); nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/24089/20/be/src/runtime/buffered-tuple-stream.cc@1191 PS20, Line 1191: DCHECK(*fn != nullptr); After the codegen->FinalizeFunction() calls I checked we don't add a dcheck, and return an error even debug code. Codegen error usually doesn't lead to failing the query I am not sure what is the better way - a DCHECK would catch codegen failures during tests, but may lead to unintended test failures. If want to be sure that codegen is always successful during tests, we could add a DCHECK in FinalizeFunction and possible add a flag like ignore_debug_codegen_failure to be able to override it in specific tests -- 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: 20 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: Mon, 29 Jun 2026 13:28:47 +0000 Gerrit-HasComments: Yes
