You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2020/03/12 23:12:57 UTC
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15424
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
[client] Add C++ API to accept BlockBloomFilter directly
For performance reasons, ability to use its own allocator etc.,
callers may choose to supply BlockBloomFilter directly
instead of using client::KuduBloomFilter. Case in point Impala.
The allocator and BlockBloomFilter are accepted as Slice
parameters that wrap the raw pointers.
This API is marked Advanced/Unstable as supplying incorrect/incompatible
parameters can lead to undefined results and it could change in
future in backward incompatible manner.
Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
5 files changed, 315 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/15424/1
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15424/3/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15424/3/src/kudu/client/client.cc@1035
PS3, Line 1035: for (auto& bf_slice : bloom_filters) {
const auto?
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Mar 2020 18:22:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15424/3/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15424/3/src/kudu/client/client.cc@1035
PS3, Line 1035: for (auto& bf_slice : bloom_filters) {
> const auto?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Mar 2020 21:53:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Mar 2020 22:11:44 +0000
Gerrit-HasComments: No
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kudu Jenkins, Adar Dembo, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15424
to look at the new patch set (#3).
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
[client] Add C++ API to accept BlockBloomFilter directly
For performance reasons, ability to use its own allocator etc.,
callers may choose to supply BlockBloomFilter directly
instead of using client::KuduBloomFilter. Case in point Impala.
The allocator and BlockBloomFilter are accepted as Slice
parameters that wrap the raw pointers.
This API is marked Advanced/Unstable as supplying incorrect/incompatible
parameters can lead to undefined results and it could change in
future in backward incompatible manner.
Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
5 files changed, 321 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/15424/3
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
[client] Add C++ API to accept BlockBloomFilter directly
For performance reasons, ability to use its own allocator etc.,
callers may choose to supply BlockBloomFilter directly
instead of using client::KuduBloomFilter. Case in point Impala.
The allocator and BlockBloomFilter are accepted as Slice
parameters that wrap the raw pointers.
This API is marked Advanced/Unstable as supplying incorrect/incompatible
parameters can lead to undefined results and it could change in
future in backward incompatible manner.
Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Reviewed-on: http://gerrit.cloudera.org:8080/15424
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
5 files changed, 321 insertions(+), 56 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
Wenzhe Zhou: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc@1025
PS1, Line 1025: reinterpret_cast<BlockBloomFilterBufferAllocatorIf*>(allocator.mutable_data());
> Done. Though one downside is changing types of allocator and bloom_filters
Hmm, given that, the earlier approach was probably better, as we're not mutating the Slice (or the vector), so cref conveys the correct semantics to the user. Sorry; didn't realize you'd have to change that.
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Mar 2020 22:22:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Mar 2020 19:05:27 +0000
Gerrit-HasComments: No
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc@1025
PS1, Line 1025: reinterpret_cast<BlockBloomFilterBufferAllocatorIf*>(allocator.mutable_data());
> Hmm, given that, the earlier approach was probably better, as we're not mut
Ack
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Mar 2020 16:12:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kudu Jenkins, Adar Dembo, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15424
to look at the new patch set (#2).
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
[client] Add C++ API to accept BlockBloomFilter directly
For performance reasons, ability to use its own allocator etc.,
callers may choose to supply BlockBloomFilter directly
instead of using client::KuduBloomFilter. Case in point Impala.
The allocator and BlockBloomFilter are accepted as Slice
parameters that wrap the raw pointers.
This API is marked Advanced/Unstable as supplying incorrect/incompatible
parameters can lead to undefined results and it could change in
future in backward incompatible manner.
Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
5 files changed, 317 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/15424/2
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc@1025
PS1, Line 1025: reinterpret_cast<BlockBloomFilterBufferAllocatorIf*>(const_cast<uint8_t*>(allocator.data()));
You can call mutable_data() instead of data(); it does the const_cast for you.
Below too.
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/scan_predicate-internal.h
File src/kudu/client/scan_predicate-internal.h:
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/scan_predicate-internal.h@147
PS1, Line 147: // This class is designed to accept BlockBloomFilter directly as raw pointer
: // from consumers like Impala. In such a case, the data is owned by the caller and not
: // by the instance of this class. So storing raw pointers and not destructing the pointers would
: // have worked fine. However for the case when predicate data is Clone()'d the internal data
: // is owned by the instance of this class. Hence using smart pointers with custom deleter
: // to keep track of ownership.
This is a useful comment, but I think it'd be better placed on DirectBloomFilterDataDeleter, to explain why it's this weird deleter with a runtime switch for no-oping.
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Mar 2020 04:29:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kudu Jenkins, Adar Dembo, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15424
to look at the new patch set (#4).
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
[client] Add C++ API to accept BlockBloomFilter directly
For performance reasons, ability to use its own allocator etc.,
callers may choose to supply BlockBloomFilter directly
instead of using client::KuduBloomFilter. Case in point Impala.
The allocator and BlockBloomFilter are accepted as Slice
parameters that wrap the raw pointers.
This API is marked Advanced/Unstable as supplying incorrect/incompatible
parameters can lead to undefined results and it could change in
future in backward incompatible manner.
Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
5 files changed, 321 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/15424/4
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 1:
> Patch Set 1: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/20809/ : FAILURE
Unrelated test failure HmsSentryConfigurations/MasterFailoverTest.TestMasterPermanentFailure
http://jenkins.kudu.apache.org/job/kudu-gerrit/20809/BUILD_TYPE=RELEASE/testReport/junit/(root)/HmsSentryConfigurations_MasterFailoverTest/TestMasterPermanentFailure_1/
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Mar 2020 23:58:31 +0000
Gerrit-HasComments: No
[kudu-CR] [client] Add C++ API to accept BlockBloomFilter directly
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc@1025
PS1, Line 1025: reinterpret_cast<BlockBloomFilterBufferAllocatorIf*>(const_cast<uint8_t*>(allocator.data()));
> You can call mutable_data() instead of data(); it does the const_cast for y
Done. Though one downside is changing types of allocator and bloom_filters to pass-by-value.
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/scan_predicate-internal.h
File src/kudu/client/scan_predicate-internal.h:
http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/scan_predicate-internal.h@147
PS1, Line 147: // This class is designed to accept BlockBloomFilter directly as raw pointer
: // from consumers like Impala. In such a case, the data is owned by the caller and not
: // by the instance of this class. So storing raw pointers and not destructing the pointers would
: // have worked fine. However for the case when predicate data is Clone()'d the internal data
: // is owned by the instance of this class. Hence using smart pointers with custom deleter
: // to keep track of ownership.
> This is a useful comment, but I think it'd be better placed on DirectBloomF
Ideally this class shouldn't take ownership. This comment explains why Clone()'ing flips the ownership from the caller to this class. So it's best kept close to the internal data structures in this class.
So instead of moving this comment, I added a line for the deleter above and linked the two.
Other option I explored is moving the deleter inside and making it a nested struct. Despite shortening the name of the deleter, from the caller perspective in client.cc the name with scope resolution appears way too long. Since this is internal header and not exposed I don't see a strong need to make the deleter nested struct.
--
To view, visit http://gerrit.cloudera.org:8080/15424
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b
Gerrit-Change-Number: 15424
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Mar 2020 20:17:40 +0000
Gerrit-HasComments: Yes