Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8041 )
Change subject: KUDU-2055 [part 2]: Add util to construct sorted disjoint interval list ...................................................................... Patch Set 7: (7 comments) Just some nits http://gerrit.cloudera.org:8080/#/c/8041/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8041/7//COMMIT_MSG@15 PS7, Line 15: For example, given the input interval list: : |------2-------| |-----1-----| : |--3--| |---5--| |----4----| : : The output sorted disjoint interval list is: : |----------1----------| |-----2-----| nit: update to [--------), here and elsewhere? http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list-test.cc File src/kudu/util/sorted_disjoint_interval_list-test.cc: http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list-test.cc@45 PS7, Line 45: a nit: "an" here and below http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list-test.cc@57 PS7, Line 57: {{4, 7}, {3, 4}, {1, 2}, {-23, 1}}; nit: Since the input is unsorted, at first glance, it wasn't clear to me how this was different from the below test. maybe comment above here too: "Coalesce adjacent intervals."? http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list.h File src/kudu/util/sorted_disjoint_interval_list.h: http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list.h@43 PS7, Line 43: |------2-------| |-----1-----| : // |--3--| |---5--| |----4----| : // : // The output sorted disjoint interval list: : // : // |----------1----------| |-----2-----| [---) ? http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list.h@61 PS7, Line 61: auto& nit: const auto&? http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list.h@81 PS7, Line 81: else { : ++head; : // Fill the gap between 'head' and 'tail', if they are not adjacent. nit: maybe move this comment just above the "++head;" and reword it to describe why this is necessary? Something like: "The two intervals are disjoint. If the 'head' previously coalesced some intervals, 'head' and 'tail' will not be adjacent. If so, make sure the next 'head' does not include any of the previously-coalesced intervals." http://gerrit.cloudera.org:8080/#/c/8041/7/src/kudu/util/sorted_disjoint_interval_list.h@85 PS7, Line 85: ++tail; nit: maybe pull this into the while loop condition? eg: while (++tail != intervals->end()) { ... } That way we could get rid of this line, L74, and L80. Although might be worse for readability. Up to you. -- To view, visit http://gerrit.cloudera.org:8080/8041 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61a813c047be4882f246eaf404598e7e18fcac87 Gerrit-Change-Number: 8041 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 27 Sep 2017 01:38:56 +0000 Gerrit-HasComments: Yes
