Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10679 )
Change subject: IMPALA-3816: (prep) Move TupleSorter to sorter-ir.cc ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-internal.h File be/src/runtime/sorter-internal.h: http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-internal.h@392 PS3, Line 392: // nit: /// http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-internal.h@396 PS3, Line 396: // nit: /// http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-internal.h@470 PS3, Line 470: /// if 'lhs' is less than 'rhs'. May need to add a remark in the follow up patch that comparator_.Less() is replaced with handcrafted IR during codegen. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-internal.h@485 PS3, Line 485: const Tuple* pivot, TupleIterator* cut); WARN_UNUSED_RESULT http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc File be/src/runtime/sorter-ir.cc: http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@209 PS3, Line 209: Tuple* Sorter::TupleSorter::MedianOfThree(Tuple* t1, Tuple* t2, Tuple* t3) { > This calls Less(). After my change Less() will always call CompareInterpret This makes sense. Missed seeing the Less() below in the first pass. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter.cc@681 PS3, Line 681: Status : Sorter::Run::AddIntermediateBatch(RowBatch* batch, int start_index, int* num_processed) { nit: we usually break the argument lists into multiple rows if it doesn't fit in a single row. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter.cc@749 PS3, Line 749: nit: indent off -- To view, visit http://gerrit.cloudera.org:8080/10679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaaf2b75c2f789002c42939865c018f728d29a113 Gerrit-Change-Number: 10679 Gerrit-PatchSet: 3 Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 14 Jun 2018 20:37:56 +0000 Gerrit-HasComments: Yes
