You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/03/25 21:11:03 UTC
[kudu-CR] rowblock: improve GetSelectedRows API
Hello Andrew Wong, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15559
to review the following change.
Change subject: rowblock: improve GetSelectedRows API
......................................................................
rowblock: improve GetSelectedRows API
The earlier commit used a boolean return value to indicate whether all
rows were selected, but as I used the API more, I found the code
somewhat confusing. This adds a wrapper class with appropriate getters
and error checking.
Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
---
M src/kudu/common/rowblock-test.cc
M src/kudu/common/rowblock.cc
M src/kudu/common/rowblock.h
M src/kudu/common/wire_protocol.cc
4 files changed, 89 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/15559/1
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Mar 2020 14:09:43 +0000
Gerrit-HasComments: No
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15559/3/src/kudu/common/rowblock.h
File src/kudu/common/rowblock.h:
http://gerrit.cloudera.org:8080/#/c/15559/3/src/kudu/common/rowblock.h@254
PS3, Line 254: &&
> Is this intentional? Does this have some special semantics?
yep, it means that this function must be called on an instance that is an rvalue (eg a temporary or the result of a std::move() call). I did this because this function essentially "breaks" the internal state of the instance by taking its 'indexes_' member out of it, so you don't want someone to continue to use the instance after the call. See http://codexpert.ro/blog/2014/10/17/c-gems-ref-qualifiers/
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 27 Mar 2020 04:14:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15559/2/src/kudu/common/rowblock.h
File src/kudu/common/rowblock.h:
http://gerrit.cloudera.org:8080/#/c/15559/2/src/kudu/common/rowblock.h@231
PS2, Line 231: bool all_selected() const {
: return all_selected_;
: }
:
: int num_selected() const {
: return all_selected_ ? sel_vector_->nrows() : indexes_.size();
: }
:
: // Get the selected rows.
: //
: // NOTE: callers must check all_selected() first, and not use this
: // function if all rows are selected. 'AsRowIndexes()' may be used instead.
: const std::vector<uint16_t>& indexes() const {
: DCHECK(!all_selected_);
: return indexes_;
: }
:
: const uint8_t* bitmap() const {
: return sel_vector_->bitmap();
: }
Are these useful outside of tests? If not, make add friend tests and make these private? Or will the be more publicly useful with your columnar patches?
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 25 Mar 2020 22:49:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15559/3/src/kudu/common/rowblock.h
File src/kudu/common/rowblock.h:
http://gerrit.cloudera.org:8080/#/c/15559/3/src/kudu/common/rowblock.h@254
PS3, Line 254: &&
Is this intentional? Does this have some special semantics?
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Mar 2020 01:11:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Mar 2020 22:08:22 +0000
Gerrit-HasComments: No
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15559/2/src/kudu/common/rowblock.h
File src/kudu/common/rowblock.h:
http://gerrit.cloudera.org:8080/#/c/15559/2/src/kudu/common/rowblock.h@231
PS2, Line 231: bool all_selected() const {
: return all_selected_;
: }
:
: int num_selected() const {
: return all_selected_ ? sel_vector_->nrows() : indexes_.size();
: }
:
: // Get the selected rows.
: //
: // NOTE: callers must check all_selected() first, and not use this
: // function if all rows are selected. 'AsRowIndexes()' may be used instead.
: const std::vector<uint16_t>& indexes() const {
: DCHECK(!all_selected_);
: return indexes_;
: }
:
: const uint8_t* bitmap() const {
: return sel_vector_->bitmap();
: }
> Are these useful outside of tests? If not, make add friend tests and make t
Ah, looks like they are.
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 25 Mar 2020 23:10:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] rowblock: improve GetSelectedRows API
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15559 )
Change subject: rowblock: improve GetSelectedRows API
......................................................................
rowblock: improve GetSelectedRows API
The earlier commit used a boolean return value to indicate whether all
rows were selected, but as I used the API more, I found the code
somewhat confusing. This adds a wrapper class with appropriate getters
and error checking.
Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Reviewed-on: http://gerrit.cloudera.org:8080/15559
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/common/rowblock-test.cc
M src/kudu/common/rowblock.cc
M src/kudu/common/rowblock.h
M src/kudu/common/wire_protocol.cc
4 files changed, 89 insertions(+), 33 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15559
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I34630c7bb854cca572ec081acbc05f3dac4db931
Gerrit-Change-Number: 15559
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>