Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/18184 )
Change subject: IMPALA-10961: Implementing adaptive 3-way quicksort in sorter ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/18184/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18184/7//COMMIT_MSG@9 PS7, Line 9: 3way > nit: 3-way Done http://gerrit.cloudera.org:8080/#/c/18184/7//COMMIT_MSG@23 PS7, Line 23: select * order by l_linestatus, NDV=2: : original 2-way vs adaptive quicksort - 1 : 0.67 : select l_shipmode order by l_shipmode, NDV=7: : original 2-way vs adaptive quicksort - 1 : 0.42 : select * order by l_shipmode, NDV=7: : original 2-way vs adaptive quicksort - 1 : 0.57 : large NDV, unique data: no significant difference > Using https://ozh.github.io/ascii-tables/ Thank you for the suggestion. Should I write the actual (average) execution time in ms or is the ratio better to compare the numbers? http://gerrit.cloudera.org:8080/#/c/18184/7/be/src/runtime/sorter-internal.h File be/src/runtime/sorter-internal.h: http://gerrit.cloudera.org:8080/#/c/18184/7/be/src/runtime/sorter-internal.h@533 PS7, Line 533: hasEquals > nit: we name variables with underscores instead of camel case in the backen Done http://gerrit.cloudera.org:8080/#/c/18184/7/be/src/runtime/sorter-ir.cc File be/src/runtime/sorter-ir.cc: http://gerrit.cloudera.org:8080/#/c/18184/7/be/src/runtime/sorter-ir.cc@157 PS7, Line 157: reinterpret_cast<TupleRow*>(&temp_tuple) > nit: Since it's being used at multiple places, maybe create a variable 'tem Good idea, makes the code more readable. I just followed the original naming convention. I think it was named temp_tuple instead of pivot to avoid confusion that it is not the actual pivot's tuple. I renamed it in Partition2way() as well. http://gerrit.cloudera.org:8080/#/c/18184/7/be/src/runtime/sorter-ir.cc@260 PS7, Line 260: hasEquals > nit: should be 'has_equals' Done http://gerrit.cloudera.org:8080/#/c/18184/7/be/src/runtime/sorter-ir.cc@320 PS7, Line 320: lt > Maybe rename it to 't1_cmp_t2'? makes sense to me, done -- To view, visit http://gerrit.cloudera.org:8080/18184 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I81e7b36a04a43de3b83e6aeee49ca0943f0bf202 Gerrit-Change-Number: 18184 Gerrit-PatchSet: 7 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 11 Feb 2022 14:25:39 +0000 Gerrit-HasComments: Yes
