Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11381 )
Change subject: KUDU-2566: Enhance rowset tree pruning and discard string copying while querying ...................................................................... Patch Set 5: (21 comments) http://gerrit.cloudera.org:8080/#/c/11381/5//COMMIT_MSG Commit Message: PS5: Thanks for rewriting the commit message. It's really nice now. http://gerrit.cloudera.org:8080/#/c/11381/5//COMMIT_MSG@7 PS5, Line 7: discard string copying nit: "stop copying strings" http://gerrit.cloudera.org:8080/#/c/11381/5//COMMIT_MSG@11 PS5, Line 11: and bounded nit: Could you wrap commit message lines at 72 characters? http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree-test.cc File src/kudu/tablet/rowset_tree-test.cc: http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree-test.cc@211 PS5, Line 211: string& By the convention of the Google style guide, we use references arguments only when they are const. Output parameters should be passed as pointers. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree-test.cc@218 PS5, Line 218: if (r == 0) continue I think we want coverage of the degenerate case when s1 == s2 (so the interval [s1, s2) is empty). http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree-test.cc@217 PS5, Line 217: int r = strcmp(s1.c_str(), s2.c_str()); : if (r == 0) continue; : left = r < 0 ? s1 : s2; : right = r < 0 ? s2 : s1; : break; You can simplify this a little bit to *left = std::min(s1, s2); *right = std::max(s1, s2); where I'm assuming left, right have been changed to be pointers and s1 == s2 is permissible. It'd also be fine to take no parameters and return a std::pair. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.h File src/kudu/tablet/rowset_tree.h: http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.h@90 PS5, Line 90: FindRowSetsIntersectingInterval Can you add a comment explaining what it means to pass boost::none for 'lower_bound' and 'upper_bound'? http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.h@90 PS5, Line 90: & style nit: We put the &'s and *'s for refs and pointers by the type, not the name, so const boost::optional<Slice>& lower_bound http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.h@91 PS5, Line 91: & Likewise. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc File src/kudu/tablet/rowset_tree.cc: http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@107 PS5, Line 107: & Likewise with the & placement nit. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@108 PS5, Line 108: & Likewise with the & placement nit. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@109 PS5, Line 109: const I don't think the const specifier is meaningful on a bool passed by value. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@117 PS5, Line 117: & Likewise with the & placement nit. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@118 PS5, Line 118: & Likewise with the & placement nit. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@119 PS5, Line 119: const Ditto on the const specifier. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@194 PS5, Line 194: & Likewise with the & placement nit. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@195 PS5, Line 195: & Likewise with the & placement nit. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.cc@196 PS5, Line 196: * * next to type, not parameter name. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/util/interval_tree-inl.h File src/kudu/util/interval_tree-inl.h: http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/util/interval_tree-inl.h@402 PS5, Line 402: intervals nit: interval http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/util/interval_tree-inl.h@409 PS5, Line 409: true To make this easier to understand locally, could you do one of two things: 1. Add a comment naming the parameter corresponding to true or false in the calls to Traits::compare. E.g. return Traits::compare(Traits::get_left(interval), upper_bound, /*positive_direction=*/true); 2. Add an enum with descriptive names for the two cases: enum class EndpointIfNone { POSITIVE_INFINITY, NEGATIVE_INFINITY, }; return Traits::compare(Traits::get_left(interval), upper_bound, POSITIVE_INFINITY); See also https://abseil.io/tips/94. http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/util/interval_tree-test.cc File src/kudu/util/interval_tree-test.cc: http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/util/interval_tree-test.cc@64 PS5, Line 64: *upper <= this->left I'm getting confused now about which intervals are inclusive of the right endpoint and which aren't. This Intersects method is checking for intersection of a closed interval [left, right] with a half-open interval [lower, upper) (when lower and upper are provided), right? Can we make this clearer? Maybe with different parameter names like `upper_excl` or `upper_exclusive`? -- To view, visit http://gerrit.cloudera.org:8080/11381 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 Gerrit-Change-Number: 11381 Gerrit-PatchSet: 5 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 07 Sep 2018 17:27:13 +0000 Gerrit-HasComments: Yes
