Andrew Wong has posted comments on this change. Change subject: KUDU-2055 [part 2]: Add util to construct sorted disjoint interval ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/8041/1/src/kudu/util/sorted_disjoint_interval-test.cc File src/kudu/util/sorted_disjoint_interval-test.cc: PS1, Line 50: struct DisjointIntervalComparator { : bool operator() (const ClosedInterval& first, const ClosedInterval& second) const { : if (first.left == second.left) return first.right < second.right; : return first.left < second.left; : } : }; nit: why does this need to be its own struct? why not a lambda within sort()? PS1, Line 58: IntTraits nit: consider IntIntervalTraits or something? Or just IntInterval? http://gerrit.cloudera.org:8080/#/c/8041/1/src/kudu/util/sorted_disjoint_interval.h File src/kudu/util/sorted_disjoint_interval.h: PS1, Line 17: #ifndef KUDU_UTIL_SORTED_DISJOINT_INTERVAL_H : #define KUDU_UTIL_SORTED_DISJOINT_INTERVAL_H Use #pragma once for C++11 ! PS1, Line 27: A sorted disjoint interval is a data structure which stores a set of intervals that : // are sorted and disjoint. nit: How about, "A set of sorted disjoint intervals holds a list of non-overlapping ranges." PS1, Line 33: |------1-------| |-----2-----| nit: to emphasize the sortedness of the sorted disjoint interval set, maybe flip 1 and 2? PS1, Line 46: The Traits class should have the following members: : // Traits::point_type : // a typedef for what a "point" in the range is : // : // Traits::interval_type : // a typedef for an interval : // : // static vector<interval_type> sort(const vector<interval_type>& interval) : // sort the given intervals. : // : // static bool valid(const interval_type& interval) : // return true if the interval is valid. : // : // static interval_type coalesce(const interval_type& a, const interval_type& b) : // coalesce two intervals. : // : // static bool overlap(const interval_type& a, const interval_type& b) : // return true if two intervals are overlapped. Is there a way to encapsulate this into some sort of templatized abstract base class? Not sure how easy/difficult that'd be with the templates. PS1, Line 65: template<class Traits> : class SortedDisjointInterval { Why does SortedDisjointInterval need its own class; could it just be a templatized utility method that takes in a vector of <Interval>s and returns a vector of <Interval>s? PS1, Line 66: SortedDisjointInterval nit: it's more of a SortedDisjointIntervalList -- To view, visit http://gerrit.cloudera.org:8080/8041 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I61a813c047be4882f246eaf404598e7e18fcac87 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
