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/31 19:41:46 UTC
[kudu-CR] client: add support for columnar format scan
Hello Andrew Wong, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15622
to review the following change.
Change subject: client: add support for columnar format scan
......................................................................
client: add support for columnar format scan
Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
A src/kudu/client/columnar_scan_batch.cc
A src/kudu/client/columnar_scan_batch.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
8 files changed, 401 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/15622/1
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
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] client: add support for columnar format scan
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 17:39:23 +0000
Gerrit-HasComments: No
[kudu-CR] client: add support for columnar format scan
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 1:
(3 comments)
Just looked at the new APIs.
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/CMakeLists.txt
File src/kudu/client/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/CMakeLists.txt@177
PS1, Line 177: # Headers: client
Need to add the new header here.
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/client.h@2414
PS1, Line 2414: static const uint64_t COLUMNAR_LAYOUT = 1 << 1;
Technically this flag is part of the "advanced/unstable" API (as per L2390). That what you want?
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/columnar_scan_batch.h
File src/kudu/client/columnar_scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/columnar_scan_batch.h@17
PS1, Line 17: #pragma once
No good for pre-C++11.
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 20:16:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: add support for columnar format scan
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15622
to look at the new patch set (#3).
Change subject: client: add support for columnar format scan
......................................................................
client: add support for columnar format scan
Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
A src/kudu/client/columnar_scan_batch.cc
A src/kudu/client/columnar_scan_batch.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tools/tool_action_remote_replica.cc
9 files changed, 421 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/15622/3
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] client: add support for columnar format scan
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.cc@1808
PS2, Line 1808: RETURN_NOT_OK(data_->OpenNextTablet(deadline, &blacklist));
: if (data_->data_in_open_) {
: // Avoid returning an empty batch in between tablets if we have data
: // we can return from this call.
: return NextBatch(batch_data);
: }
> nit: Even in the worst case (large table, no rows), we don't need to worry
this can't loop -- if it recurses once, the 'data_in_open_' case will trigger at the top of the function (line 1738) and return
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 17:36:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: add support for columnar format scan
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Andrew Wong, Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15622
to look at the new patch set (#2).
Change subject: client: add support for columnar format scan
......................................................................
client: add support for columnar format scan
Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
A src/kudu/client/columnar_scan_batch.cc
A src/kudu/client/columnar_scan_batch.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tools/tool_action_remote_replica.cc
9 files changed, 402 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/15622/2
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] client: add support for columnar format scan
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
client: add support for columnar format scan
Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Reviewed-on: http://gerrit.cloudera.org:8080/15622
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
A src/kudu/client/columnar_scan_batch.cc
A src/kudu/client/columnar_scan_batch.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tools/tool_action_remote_replica.cc
9 files changed, 421 insertions(+), 23 deletions(-)
Approvals:
Adar Dembo: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] client: add support for columnar format scan
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/client.h@2414
PS1, Line 2414: static const uint64_t COLUMNAR_LAYOUT = 1 << 1;
> Technically this flag is part of the "advanced/unstable" API (as per L2390)
yea that was on purpose -- I figure we might want to tweak the format a little as we write clients/integrations that use it (particularly the Slice format is not very efficient)
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Mar 2020 20:23:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: add support for columnar format scan
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.cc@1808
PS2, Line 1808: RETURN_NOT_OK(data_->OpenNextTablet(deadline, &blacklist));
: if (data_->data_in_open_) {
: // Avoid returning an empty batch in between tablets if we have data
: // we can return from this call.
: return NextBatch(batch_data);
: }
> this can't loop -- if it recurses once, the 'data_in_open_' case will trigg
Ack
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@790
PS2, Line 790:
: unique_ptr<ColumnarRowBlockPB> resp_data(response->release_columnar_data());
: if (!resp_data) {
> can't really because resp_data_ is an instance, not a pointer. So we still
Ack
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:09:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: add support for columnar format scan
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/CMakeLists.txt
File src/kudu/client/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/CMakeLists.txt@177
PS1, Line 177: # Headers: client
> Need to add the new header here.
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client-test.cc@1056
PS2, Line 1056: TEST_F(ClientTest, TestColumnarScan) {
> nit: can you explicitly set scanner_batch_size_rows too in case the default
done. randomized the num rows and the batch size
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client-test.cc@1773
PS2, Line 1773:
> nit: drop?
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.h@2270
PS2, Line 2270: /// This variant may only be used when the scan is configured with the
: /// COLUMNAR_LAYOUT RowFormatFlag.
> nit: could you add a similar warning to the other NextBatch comments?
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.h
File src/kudu/client/columnar_scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.h@18
PS2, Line 18:
> nit: drop a couple lines
Done
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/columnar_scan_batch.h
File src/kudu/client/columnar_scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/15622/1/src/kudu/client/columnar_scan_batch.h@17
PS1, Line 17: #pragma once
> No good for pre-C++11.
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.cc
File src/kudu/client/columnar_scan_batch.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.cc@37
PS2, Line 37: /// @return The number of rows in this batch.
> nit: this is already in the header?
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.h@375
PS2, Line 375: const Schema* projection,
: const KuduSchema* client_projection,
: uint64_t row_format_flags,
: tserver::ScanResponsePB* response) override;
> nit: spacing
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.h@381
PS2, Line 381: Status GetDataForColumn(int idx, Slice* data) const;
> nit: could you doc this? Probably worth calling out that even though this i
the docs are in the public KuduColumnarScanBatch API. I'll add a comment down next to 'mutable' below
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@790
PS2, Line 790:
: unique_ptr<ColumnarRowBlockPB> resp_data(response->release_columnar_data());
: if (!resp_data) {
> Can we use response->has_columnar_data() here and inline the release below?
can't really because resp_data_ is an instance, not a pointer. So we still end up with just as many lines below since we need to free the one that was released
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@808
PS2, Line 808: return Status::InvalidArgument("bad column index");
> nit: Do you think it's worth also including columns_size() here?
Done
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@815
PS2, Line 815: RETURN_NOT_OK(controller_.GetInboundSidecar(
: resp_data_.columns(idx).data_sidecar(),
: data));
> nit: Might be nice to only update 'data' if we return Status::OK. OTOH, if
yea, it's a bit weird to give this consistent semantics such that calling it multiple times on error gives the same error back. I'll add a note.
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 17:35:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: add support for columnar format scan
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15622 )
Change subject: client: add support for columnar format scan
......................................................................
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client-test.cc@1056
PS2, Line 1056: TEST_F(ClientTest, TestColumnarScan) {
nit: can you explicitly set scanner_batch_size_rows too in case the default or in case test_scan_num_rows ever changes?
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client-test.cc@1773
PS2, Line 1773:
nit: drop?
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.h@2270
PS2, Line 2270: /// This variant may only be used when the scan is configured with the
: /// COLUMNAR_LAYOUT RowFormatFlag.
nit: could you add a similar warning to the other NextBatch comments?
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/client.cc@1808
PS2, Line 1808: RETURN_NOT_OK(data_->OpenNextTablet(deadline, &blacklist));
: if (data_->data_in_open_) {
: // Avoid returning an empty batch in between tablets if we have data
: // we can return from this call.
: return NextBatch(batch_data);
: }
nit: Even in the worst case (large table, no rows), we don't need to worry about this blowing up because we're using tail recursion, right? Maybe worth calling out in a comment.
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.h
File src/kudu/client/columnar_scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.h@18
PS2, Line 18:
nit: drop a couple lines
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.cc
File src/kudu/client/columnar_scan_batch.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/columnar_scan_batch.cc@37
PS2, Line 37: /// @return The number of rows in this batch.
nit: this is already in the header?
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.h@375
PS2, Line 375: const Schema* projection,
: const KuduSchema* client_projection,
: uint64_t row_format_flags,
: tserver::ScanResponsePB* response) override;
nit: spacing
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.h@381
PS2, Line 381: Status GetDataForColumn(int idx, Slice* data) const;
nit: could you doc this? Probably worth calling out that even though this is const, it's actually rewriting some column slices.
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@790
PS2, Line 790:
: unique_ptr<ColumnarRowBlockPB> resp_data(response->release_columnar_data());
: if (!resp_data) {
Can we use response->has_columnar_data() here and inline the release below? Same above?
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@808
PS2, Line 808: return Status::InvalidArgument("bad column index");
nit: Do you think it's worth also including columns_size() here?
http://gerrit.cloudera.org:8080/#/c/15622/2/src/kudu/client/scanner-internal.cc@815
PS2, Line 815: RETURN_NOT_OK(controller_.GetInboundSidecar(
: resp_data_.columns(idx).data_sidecar(),
: data));
nit: Might be nice to only update 'data' if we return Status::OK. OTOH, if we get an error below, our data's corrupted anyway.
--
To view, visit http://gerrit.cloudera.org:8080/15622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69ec4487c78b872e7b9eba5facdd44cfd6b8f4fa
Gerrit-Change-Number: 15622
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:55:46 +0000
Gerrit-HasComments: Yes