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 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/data-stream-test.cc@452 PS9, Line 452: ///TODO: codegend_heapify_helper_fn currently stores a nullptr. Should call codegen Can we resolve this TODO in this change or should it be another commit? http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.h File be/src/runtime/sorted-run-merger.h: http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.h@132 PS9, Line 132: and Nit: wouldn't "or" be better? See also sorter.h#{261,265}. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.h@142 PS9, Line 142: class SortedRunMerger::SortedRunWrapper { Do we need to move this class here from the .cc file? I haven't found any places where we use it outside of the .cc. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc File be/src/runtime/sorted-run-merger.cc: http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@33 PS9, Line 33: NULL Nit: nullptr would be better. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@42 PS9, Line 42: NULL Nit: nullptr would be better. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@46 PS9, Line 46: NULL Nit: nullptr would be better. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@53 PS9, Line 53: NULL Nit: nullptr would be better. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@63 PS9, Line 63: NULL Nit: nullptr would be better. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@74 PS9, Line 74: NULL Nit: nullptr would be better. http://gerrit.cloudera.org:8080/#/c/18824/9/be/src/runtime/sorted-run-merger.cc@76 PS9, Line 76: NULL Nit: nullptr would be better. -- 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: 9 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: Thu, 23 Feb 2023 15:11:04 +0000 Gerrit-HasComments: Yes
