Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17869 )
Change subject: IMPALA-10956: datasketches UDFs: memory leak and merge overhead ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/17869/4/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/17869/4/be/src/exprs/aggregate-functions-ir.cc@2075 PS4, Line 2075: agg_state_ptr->second = new (ctx->Allocate<datasketches::update_theta_sketch>()) : datasketches::update_theta_sketch( : datasketches::update_theta_sketch::builder().build()) I looked through the patch again looking for potential leaks. bit I didn't find any. Theta seems the most suspicious to me because the std::uninitialized_fill_n(sketch_ptr, 1, sketch); used in the original version - I don't know what is the reason behind using it. http://gerrit.cloudera.org:8080/#/c/17869/4/be/src/exprs/aggregate-functions-ir.cc@2151 PS4, Line 2151: u.update(*dst_sketch_ptr); Couldn't we use std::move here? update seems to be optimized for the rvalue case. -- To view, visit http://gerrit.cloudera.org:8080/17869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8dd0e6736f4266f74f5f265f58d40a4e4707287f Gerrit-Change-Number: 17869 Gerrit-PatchSet: 4 Gerrit-Owner: Alexander Saydakov <[email protected]> Gerrit-Reviewer: Alexander Saydakov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 10 Dec 2021 11:35:07 +0000 Gerrit-HasComments: Yes
