Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18824 )
Change subject: IMPALA-11477: Adding Codegen to sorted-run-merger ...................................................................... Patch Set 6: (15 comments) http://gerrit.cloudera.org:8080/#/c/18824/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18824/6//COMMIT_MSG@14 PS6, Line 14: Re-writed Nit: Re-wrote http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/exchange-node.cc File be/src/exec/exchange-node.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/exchange-node.cc@141 PS6, Line 141: const ExchangePlanNode& pnode = static_cast<const ExchangePlanNode&>(plan_node_); Could be moved within the IF below to limit its scope. Also, optionally we could add a DCHECK with a dynamic cast: DCHECK(dynamic_cast<const ExchangePlanNode*>(&plan_node_) != nullptr); http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/partial-sort-node.cc File be/src/exec/partial-sort-node.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/partial-sort-node.cc@101 PS6, Line 101: AddCodegenStatus( Is there a reason why in ExchangePlanNode::Codegen() we check whether 'compare_fn' is NULL and only proceed if it isn't, but here we don't? If 'compare_fn' should never be NULL here, we could add a DCHECK checking that here. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/sort-node.cc File be/src/exec/sort-node.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/sort-node.cc@121 PS6, Line 121: SortedRunMerger::Codegen(state, compare_fn, &codegend_heapify_helper_fn_); See comment at partial-sort-node.cc:101. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/exec/topn-node.cc@225 PS6, Line 225: codegen_status = See comment at partial-sort-node.cc:101 (it also applies to Sorter::TupleSorter::Codegen on L221). http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/krpc-data-stream-recvr.h File be/src/runtime/krpc-data-stream-recvr.h: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/krpc-data-stream-recvr.h@108 PS6, Line 108: _ Parameter shouldn't end in an underscore. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/krpc-data-stream-recvr.h@196 PS6, Line 196: codegend_heapify_helper_fn_ Do we use it anywhere? In CreateMerger() we use the parameter, right? http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/krpc-data-stream-recvr.cc@643 PS6, Line 643: _ Parameter shouldn't end in an underscore. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger-ir.cc File be/src/runtime/sorted-run-merger-ir.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger-ir.cc@23 PS6, Line 23: Nit: extra space. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger-ir.cc@25 PS6, Line 25: Status We always return Status::OK(), do we need to change the return type from void to Status in this patch? http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger.cc File be/src/runtime/sorted-run-merger.cc: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger.cc@17 PS6, Line 17: //#include "runtime/row-batch.h" Remove commented lines, see also L21. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger.cc@85 PS6, Line 85: RETURN_IF_ERROR(heapify_helper_fn(this, parent_index)); Nit: you could simply return in both branches of the IF statement, no need for RETURN_IF_ERROR. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger.cc@172 PS6, Line 172: NULL Nit: nullptr is better. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorted-run-merger.cc@175 PS6, Line 175: compare_fn We could also DCHECK that 'compare_fn' is not null. http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorter.h File be/src/runtime/sorter.h: http://gerrit.cloudera.org:8080/#/c/18824/6/be/src/runtime/sorter.h@123 PS6, Line 123: _ Parameter shouldn't end with an underscore. -- To view, visit http://gerrit.cloudera.org:8080/18824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic35c7460bdbd54b8ec5872a83680e2f41ceae9fd Gerrit-Change-Number: 18824 Gerrit-PatchSet: 6 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Comment-Date: Mon, 19 Sep 2022 13:50:43 +0000 Gerrit-HasComments: Yes
