Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9943 )
Change subject: IMPALA-5706: Spilling sort optimisations ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/9943/8/be/src/runtime/sorter.h File be/src/runtime/sorter.h: http://gerrit.cloudera.org:8080/#/c/9943/8/be/src/runtime/sorter.h@193 PS8, Line 193: /// function calculates the maximum number of runs that can be taken care during the this sentence seems garbled. "can be taken care of"? http://gerrit.cloudera.org:8080/#/c/9943/8/be/src/runtime/sorter.h@199 PS8, Line 199: /// round of merging. Can you mention the invariant that we have enough reservation for this number of runs. E.g. "Returns at most MaxRunsInNextMerge(), so the sorter will have enough reservation to merge this number of runs". http://gerrit.cloudera.org:8080/#/c/9943/8/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: http://gerrit.cloudera.org:8080/#/c/9943/8/be/src/runtime/sorter.cc@1676 PS8, Line 1676: num_required_buffers += var_len_pages_found ? 2 : 1; I think we need two buffers regardless if there are var-len slots, since Run::Init() always allocates a var-len page. http://gerrit.cloudera.org:8080/#/c/9943/8/be/src/runtime/sorter.cc@1679 PS8, Line 1679: i-1 nit: i - 1 http://gerrit.cloudera.org:8080/#/c/9943/8/tests/query_test/test_sort.py File tests/query_test/test_sort.py: http://gerrit.cloudera.org:8080/#/c/9943/8/tests/query_test/test_sort.py@104 PS8, Line 104: assert "TotalMergesPerformed: 1" in query_result.runtime_profile Should we also set num_nodes=1 here? Otherwise it might not be robust when running on different clusters. -- To view, visit http://gerrit.cloudera.org:8080/9943 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74857c1694802e81f1cfc765d2b4e8bc644387f9 Gerrit-Change-Number: 9943 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 15 May 2018 23:30:40 +0000 Gerrit-HasComments: Yes
