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 22: (6 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: > Similarly to https://gerrit.cloudera.org/#/c/24132/ , it would be nice how Done 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 pat Ack 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? Done 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: ateTest, UnpinFul > Why do the codegen tests use a different tuple layout? Ideally the same tes Rewrote the changes in the test file to use both interpreted and codegen'd function in the test cases. 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 Done http://gerrit.cloudera.org:8080/#/c/24089/21/be/src/runtime/buffered-tuple-stream.cc@1254 PS21, Line 1254: // The stream should be in a consistent state once the row is added. : CHECK_CONSISTENCY_FAST(read_it_); : } : : bool IR_NO_INLINE BufferedTupleStream::DeepCopy( : TupleRow* row, uint8_t** data, const uint8_t* data_end) noexcept { : return has_nullable_tuple_ ? DeepCopyInternal<true>(row, data, data_end) : : DeepCopyInternal<false>(row, data, data_end); : } : : // TODO: in case of duplicate tuples, this can redundantly serialize data. : template <bool HAS_NULLABLE_TUPLE> : bool BufferedTupleStream::DeepCopyInternal( : TupleRow* row, uint8_t** data, const uint8_t* data_end) noexcept { : uint8_t* pos = *data; : const > The two blocks could merged by passing HAS_NULLABLE_TUPLE as last parameter 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: 22 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, 07 Jul 2026 12:31:38 +0000 Gerrit-HasComments: Yes
