Hao Hao 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 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/8041/5/src/kudu/util/sorted_disjoint_interval_list.h File src/kudu/util/sorted_disjoint_interval_list.h: http://gerrit.cloudera.org:8080/#/c/8041/5/src/kudu/util/sorted_disjoint_interval_list.h@32 PS5, Line 32: // In-place constructing a sorted disjoint interval list given a list of intervals. > How about "Constructs a sorted disjoint interval list in-place given a list Done http://gerrit.cloudera.org:8080/#/c/8041/5/src/kudu/util/sorted_disjoint_interval_list.h@63 PS5, Line 63: IllegalState > I think IllegalArgument is more appropriate here. Done http://gerrit.cloudera.org:8080/#/c/8041/5/src/kudu/util/sorted_disjoint_interval_list.h@75 PS5, Line 75: for (; end != intervals->end();) { > nit: use a while loop? Done http://gerrit.cloudera.org:8080/#/c/8041/5/src/kudu/util/sorted_disjoint_interval_list.h@76 PS5, Line 76: overlaps > s/overlaps/overlap Done http://gerrit.cloudera.org:8080/#/c/8041/5/src/kudu/util/sorted_disjoint_interval_list.h@79 PS5, Line 79: start->second = end->second; > Shouldn't this be setting it to the larger end of the two? I think as is t Right, good catch! Thanks! http://gerrit.cloudera.org:8080/#/c/8041/4/src/kudu/util/sorted_disjoint_interval_list.h File src/kudu/util/sorted_disjoint_interval_list.h: http://gerrit.cloudera.org:8080/#/c/8041/4/src/kudu/util/sorted_disjoint_interval_list.h@64 PS4, Line 64: > It's kind of ambiguous what [0, 0) even means. I think [0, 0) should be invalid mathematically. But either let LBM to filter those blocks or drop those before coalescing them, or even just consider it to be valid and do nothing, seems to be equivalent from the output perspective. So I will just stick to the third option (just consider it to be valid) if no objections. -- 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: 5 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 22 Sep 2017 22:18:33 +0000 Gerrit-HasComments: Yes
