[kudu-CR] KUDU-2566: Enhance rowset tree pruning and stop copying strings
helifu has posted comments on this change. (
http://gerrit.cloudera.org:8080/11381 )
Change subject: KUDU-2566: Enhance rowset tree pruning and stop copying strings
..
Patch Set 9:
(47 comments)
thanks to Adar and Will.
http://gerrit.cloudera.org:8080/#/c/11381/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11381/2//COMMIT_MSG@7
PS2, Line 7: Enhance rowset tr
> Could you make the subject more descriptive? Howabout "[tablet] KUDU-2566:
Done
http://gerrit.cloudera.org:8080/#/c/11381/2//COMMIT_MSG@10
PS2, Line 10: Support open-ended intervals
> Following good commit message guidelines, could you explain what the previo
Done
http://gerrit.cloudera.org:8080/#/c/11381/2//COMMIT_MSG@11
PS2, Line 11: Previously, the rowset tree could only compute overlap with a
> Again, let's fill out the explanation here a bit. My suggestion (adding on
Done
http://gerrit.cloudera.org:8080/#/c/11381/5//COMMIT_MSG
Commit Message:
PS5:
> Thanks for rewriting the commit message. It's really nice now.
Done
http://gerrit.cloudera.org:8080/#/c/11381/5//COMMIT_MSG@7
PS5, Line 7: stop copying strings
> nit: "stop copying strings"
Done
http://gerrit.cloudera.org:8080/#/c/11381/5//COMMIT_MSG@11
PS5, Line 11:
> nit: Could you wrap commit message lines at 72 characters?
Done
http://gerrit.cloudera.org:8080/#/c/11381/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11381/6//COMMIT_MSG@8
PS6, Line 8:
> I think git only uses the first line as the subject so inserting a line bre
Done
http://gerrit.cloudera.org:8080/#/c/11381/4/src/kudu/tablet/rowset_tree-test.cc
File src/kudu/tablet/rowset_tree-test.cc:
http://gerrit.cloudera.org:8080/#/c/11381/4/src/kudu/tablet/rowset_tree-test.cc@18
PS4, Line 18: #include
> warning: #includes are not sorted properly [llvm-include-order]
Done
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: domized
> By the convention of the Google style guide, we use references arguments on
Done
http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree-test.cc@218
PS5, Line 218:
> I think we want coverage of the degenerate case when s1 == s2 (so the inter
Done
http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree-test.cc@217
PS5, Line 217: UND_EQUAL
: };
: const auto& GetStringPair = [] (const BoundOperator op) ->
std::pair {
: while (true) {
: string
> You can simplify this a little bit to
Done
http://gerrit.cloudera.org:8080/#/c/11381/6/src/kudu/tablet/rowset_tree-test.cc
File src/kudu/tablet/rowset_tree-test.cc:
http://gerrit.cloudera.org:8080/#/c/11381/6/src/kudu/tablet/rowset_tree-test.cc@251
PS6, Line 251: vector out;
> Don't need to SeedRandom() twice in a test.
Done
http://gerrit.cloudera.org:8080/#/c/11381/6/src/kudu/tablet/rowset_tree-test.cc@283
PS6, Line 283: testing::Values(10, 100, 250, 500),
> Should either fix this test case, or remove it.
Done
http://gerrit.cloudera.org:8080/#/c/11381/6/src/kudu/tablet/rowset_tree.h
File src/kudu/tablet/rowset_tree.h:
http://gerrit.cloudera.org:8080/#/c/11381/6/src/kudu/tablet/rowset_tree.h@96
PS6, Line 96: // [lower_bound, upper_bound)
> Nit: got a tab here.
Done
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: en 'lower_bound' is boost::none
> Can you add a comment explaining what it means to pass boost::none for 'low
Done
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 th
Done
http://gerrit.cloudera.org:8080/#/c/11381/5/src/kudu/tablet/rowset_tree.h@91
PS5, Line 91:
> Likewise.
Done
http://gerrit.cloudera.org:8080/#/c/11381/2/src/kudu/tablet/rowset_tree.h
File src/kudu/tablet/rowset_tree.h:
http://gerrit.cloudera.org:8080/#/c/11381/2/src/kudu/tablet/rowset_tree.h@88
PS2, Line 88:const
std::function& cb) const;
:
: // When 'lower_bound' is boost::none, it means negative
infinity.
: // When 'upper_bound' is boost::none, it means positive
infinity.
: // So the query interval can be one of below:
: // [-OO, +OO)
: // [-OO, upper_bound)
> +1. We could also accept boost::none for both arguments and short-circuit r
some tests, example TestTablet.TestRowIteratorSimple in tablet-test.cc, will
rely on rowsets's sequence, so we need keep the original logic.
http://gerrit.cloudera.org:8080/#/c/11381/2/src/kudu/tablet/rowset_tree.h@88
PS2, Line 88:
[kudu-CR] KUDU-2566: Enhance rowset tree pruning and stop copying strings
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11381 ) Change subject: KUDU-2566: Enhance rowset tree pruning and stop copying strings .. KUDU-2566: Enhance rowset tree pruning and stop copying strings Three improvements: 1) Support open-ended intervals: Previously, the rowset tree could only compute overlap with a closed and bounded interval. This changeset adds the ability to find overlap for unbounded intervals as well. As a result, tablet iterators with a single primary key bound are more efficient because they do not iterate over rowsets that don't overlap with the key bound. 2) End up fetching one more rowset for the upper bound which is exclusive: This changeset also adds the ability to query the rowset tree using an exclusive upper bound, whereas previously only inclusive intervals were supported. This makes scans more efficient since primary key upper bounds are passed as exclusive bounds, so now rowsets that overlap only in the upper bound are excluded. 3) Perf improvement: Using raw slices instead of copying to strings while querying. The copying from slices to string is discarded, which could help to increase the query performance. Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 Reviewed-on: http://gerrit.cloudera.org:8080/11381 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/tablet/rowset_tree-test.cc M src/kudu/tablet/rowset_tree.cc M src/kudu/tablet/rowset_tree.h M src/kudu/tablet/tablet.cc M src/kudu/util/interval_tree-inl.h M src/kudu/util/interval_tree-test.cc M src/kudu/util/interval_tree.h 7 files changed, 352 insertions(+), 85 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 Gerrit-Change-Number: 11381 Gerrit-PatchSet: 9 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2566: Enhance rowset tree pruning and stop copying strings
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11381 ) Change subject: KUDU-2566: Enhance rowset tree pruning and stop copying strings .. Patch Set 8: Code-Review+2 -- 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: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 11 Sep 2018 16:24:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-2566: Enhance rowset tree pruning and stop copying strings
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11381 to look at the new patch set (#8). Change subject: KUDU-2566: Enhance rowset tree pruning and stop copying strings .. KUDU-2566: Enhance rowset tree pruning and stop copying strings Three improvements: 1) Support open-ended intervals: Previously, the rowset tree could only compute overlap with a closed and bounded interval. This changeset adds the ability to find overlap for unbounded intervals as well. As a result, tablet iterators with a single primary key bound are more efficient because they do not iterate over rowsets that don't overlap with the key bound. 2) End up fetching one more rowset for the upper bound which is exclusive: This changeset also adds the ability to query the rowset tree using an exclusive upper bound, whereas previously only inclusive intervals were supported. This makes scans more efficient since primary key upper bounds are passed as exclusive bounds, so now rowsets that overlap only in the upper bound are excluded. 3) Perf improvement: Using raw slices instead of copying to strings while querying. The copying from slices to string is discarded, which could help to increase the query performance. Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 --- M src/kudu/tablet/rowset_tree-test.cc M src/kudu/tablet/rowset_tree.cc M src/kudu/tablet/rowset_tree.h M src/kudu/tablet/tablet.cc M src/kudu/util/interval_tree-inl.h M src/kudu/util/interval_tree-test.cc M src/kudu/util/interval_tree.h 7 files changed, 352 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/11381/8 -- 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: newpatchset Gerrit-Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 Gerrit-Change-Number: 11381 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2566: Enhance rowset tree pruning and stop copying strings
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11381 ) Change subject: KUDU-2566: Enhance rowset tree pruning and stop copying strings .. Patch Set 7: (3 comments) Couple more nits and this is good. http://gerrit.cloudera.org:8080/#/c/11381/7/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/11381/7/src/kudu/tablet/tablet.cc@1793 PS7, Line 1793: // It's a little bit difficult to merge the logic below to upper, : // because some test cases reply on it :( Can remove this. http://gerrit.cloudera.org:8080/#/c/11381/7/src/kudu/tablet/tablet.cc@2362 PS7, Line 2362: nit: extra line http://gerrit.cloudera.org:8080/#/c/11381/7/src/kudu/util/interval_tree-test.cc File src/kudu/util/interval_tree-test.cc: http://gerrit.cloudera.org:8080/#/c/11381/7/src/kudu/util/interval_tree-test.cc@187 PS7, Line 187: & nit: Here and below, & with type. -- 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: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 11 Sep 2018 03:05:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2566: Enhance rowset tree pruning and stop copying strings
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11381 to look at the new patch set (#7). Change subject: KUDU-2566: Enhance rowset tree pruning and stop copying strings .. KUDU-2566: Enhance rowset tree pruning and stop copying strings Three improvements: 1) Support open-ended intervals: Previously, the rowset tree could only compute overlap with a closed and bounded interval. This changeset adds the ability to find overlap for unbounded intervals as well. As a result, tablet iterators with a single primary key bound are more efficient because they do not iterate over rowsets that don't overlap with the key bound. 2) End up fetching one more rowset for the upper bound which is exclusive: This changeset also adds the ability to query the rowset tree using an exclusive upper bound, whereas previously only inclusive intervals were supported. This makes scans more efficient since primary key upper bounds are passed as exclusive bounds, so now rowsets that overlap only in the upper bound are excluded. 3) Perf improvement: Using raw slices instead of copying to strings while querying. The copying from slices to string is discarded, which could help to increase the query performance. Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 --- M src/kudu/tablet/rowset_tree-test.cc M src/kudu/tablet/rowset_tree.cc M src/kudu/tablet/rowset_tree.h M src/kudu/tablet/tablet.cc M src/kudu/util/interval_tree-inl.h M src/kudu/util/interval_tree-test.cc M src/kudu/util/interval_tree.h 7 files changed, 343 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/11381/7 -- 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: newpatchset Gerrit-Change-Id: I0e34b4169f93f3519f694c9bf86ca5295c318e79 Gerrit-Change-Number: 11381 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
