Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch() ...................................................................... Patch Set 24: (5 comments) Looking good, just had a few nits! http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h File be/src/util/priority-queue.h: http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h@124 PS21, Line 124: > nit: add spaces around operator. *clang-format http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h@51 PS23, Line 51: llvm::Function*& compare_fn nit: we usually avoid references for output arguments (cause it makes the fact that it's potentially mutated at the callsite less obvious), so this should be a ** http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h@56 PS23, Line 56: = nit: formatting http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h@164 PS23, Line 164: versoin nit: version http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.cc File be/src/util/tuple-row-compare.cc: http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.cc@83 PS23, Line 83: //llvm::Function* fn; Remove this? -- To view, visit http://gerrit.cloudera.org:8080/16373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8 Gerrit-Change-Number: 16373 Gerrit-PatchSet: 24 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 10 Sep 2020 21:23:05 +0000 Gerrit-HasComments: Yes
