This is an automated email from the ASF dual-hosted git repository.
bankim pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new d652cab [client] Remove allocator param for direct Bloom filter
predicate
d652cab is described below
commit d652cab17d52bc7959b6766c2e1cc9db7807e2a5
Author: Bankim Bhavsar <[email protected]>
AuthorDate: Mon May 18 15:20:52 2020 -0700
[client] Remove allocator param for direct Bloom filter predicate
Direct Bloom filter predicate in kudu client API is meant for consumption by
Impala.
Supplying the allocator that's associated with Bloom filter is turning out
to
be tricky for Impala. For instance, it doesn't want to support Clone() and
concerned with the allocator being used by kudu client for
allocation/deallocation of memory.
Allocator associated with the Bloom filter is only used for cloning a
predicate
and the support for cloning is only used for internal testing purposes.
So this change removes the allocator parameter from the direct Bloom filter
predicate API. For cloning the direct Bloom filter, it uses the default
allocator instead.
Since this API is marked as advanced/unstable, no compatibility concerns.
Change-Id: I7b57a6cacc8a5b6243787df8f896b74aa04f3916
Reviewed-on: http://gerrit.cloudera.org:8080/15947
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <[email protected]>
---
src/kudu/client/client.cc | 28 +++++++++-------------------
src/kudu/client/client.h | 4 ----
src/kudu/client/predicate-test.cc | 7 ++-----
src/kudu/client/scan_predicate-internal.h | 3 ---
src/kudu/client/scan_predicate.cc | 16 +++++++---------
5 files changed, 18 insertions(+), 40 deletions(-)
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index 4a97e28..ea35f71 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -89,7 +89,6 @@
#include "kudu/tserver/tserver.pb.h"
#include "kudu/tserver/tserver_service.proxy.h"
#include "kudu/util/async_util.h"
-#include "kudu/util/block_bloom_filter.h"
#include "kudu/util/debug-util.h"
#include "kudu/util/init.h"
#include "kudu/util/logging.h"
@@ -100,6 +99,10 @@
#include "kudu/util/scoped_cleanup.h"
#include "kudu/util/version_info.h"
+namespace kudu {
+class BlockBloomFilter;
+} // namespace kudu
+
using kudu::consensus::RaftPeerPB;
using kudu::master::AlterTableRequestPB;
using kudu::master::AlterTableResponsePB;
@@ -1006,7 +1009,6 @@ KuduPredicate* KuduTable::NewInBloomFilterPredicate(const
Slice& col_name,
}
KuduPredicate* KuduTable::NewInBloomFilterPredicate(const Slice& col_name,
- const Slice& allocator,
const vector<Slice>&
bloom_filters) {
// Empty vector of bloom filters will select all rows. Hence disallowed.
if (bloom_filters.empty()) {
@@ -1014,26 +1016,15 @@ KuduPredicate*
KuduTable::NewInBloomFilterPredicate(const Slice& col_name,
new ErrorPredicateData(Status::InvalidArgument("No Bloom filters
supplied")));
}
- // In this case, the Block Bloom filters and allocator are supplied as
opaque pointers
- // and the predicate will convert them to well-defined pointer types
- // but will NOT take ownership of those pointers. Hence a custom deleter,
- // DirectBloomFilterDataDeleter, is used that gives control over ownership.
-
- // Extract the allocator.
- auto* bbf_allocator =
- // const_cast<> can be avoided with mutable_data() on a Slice. However
mutable_data() is a
- // non-const function which can't be used with the const allocator Slice.
- // Same for BlockBloomFilter below.
-
reinterpret_cast<BlockBloomFilterBufferAllocatorIf*>(const_cast<uint8_t*>(allocator.data()));
- std::shared_ptr<BlockBloomFilterBufferAllocatorIf> bbf_allocator_shared(
- bbf_allocator,
- DirectBloomFilterDataDeleter<BlockBloomFilterBufferAllocatorIf>(false
/*owned*/));
-
// Extract the Block Bloom filters.
vector<DirectBlockBloomFilterUniqPtr> bbf_vec;
for (const auto& bf_slice : bloom_filters) {
auto* bbf =
reinterpret_cast<BlockBloomFilter*>(const_cast<uint8_t*>(bf_slice.data()));
+ // In this case, the Block Bloom filters are supplied as opaque pointers
+ // and the predicate will convert them to well-defined pointer types
+ // but will NOT take ownership of those pointers. Hence a custom deleter,
+ // DirectBloomFilterDataDeleter, is used that gives control over ownership.
DirectBlockBloomFilterUniqPtr bf_uniq_ptr(
bbf, DirectBloomFilterDataDeleter<BlockBloomFilter>(false /*owned*/));
bbf_vec.emplace_back(std::move(bf_uniq_ptr));
@@ -1041,8 +1032,7 @@ KuduPredicate* KuduTable::NewInBloomFilterPredicate(const
Slice& col_name,
return data_->MakePredicate(col_name, [&](const ColumnSchema& col_schema) {
return new KuduPredicate(
- new InDirectBloomFilterPredicateData(col_schema,
std::move(bbf_allocator_shared),
- std::move(bbf_vec)));
+ new InDirectBloomFilterPredicateData(col_schema, std::move(bbf_vec)));
});
}
diff --git a/src/kudu/client/client.h b/src/kudu/client/client.h
index f533e57..82ab546 100644
--- a/src/kudu/client/client.h
+++ b/src/kudu/client/client.h
@@ -1137,9 +1137,6 @@ class KUDU_EXPORT KuduTable : public
sp::enable_shared_from_this<KuduTable> {
///
/// @param [in] col_name
/// Name of the column to which the predicate applies.
- /// @param [in] allocator
- /// Pointer to the BlockBloomFilterBufferAllocatorIf allocator used with
- /// Bloom filters.
/// @param bloom_filters
/// Vector of BlockBloomFilter pointers that contain the values inserted to
/// match against the column. The column value must match against all the
@@ -1155,7 +1152,6 @@ class KUDU_EXPORT KuduTable : public
sp::enable_shared_from_this<KuduTable> {
/// a non-NULL value is still returned. The error will be returned when
/// attempting to add this predicate to a KuduScanner.
KuduPredicate* NewInBloomFilterPredicate(const Slice& col_name,
- const Slice& allocator,
const std::vector<Slice>&
bloom_filters);
///@}
diff --git a/src/kudu/client/predicate-test.cc
b/src/kudu/client/predicate-test.cc
index efc49f7..3d17c8a 100644
--- a/src/kudu/client/predicate-test.cc
+++ b/src/kudu/client/predicate-test.cc
@@ -1413,15 +1413,12 @@ TEST_F(BloomFilterPredicateTest,
TestDirectBlockBloomFilterPredicate) {
InsertAllValuesInTable();
- auto* allocator = DefaultBlockBloomFilterBufferAllocator::GetSingleton();
- Slice allocator_slice(reinterpret_cast<const uint8_t*>(allocator),
sizeof(*allocator));
-
vector<Slice> included_bf_vec =
{ Slice(reinterpret_cast<const uint8_t*>(included_bf.get()),
sizeof(*included_bf)) };
const size_t included_bf_vec_size = included_bf_vec.size();
auto* included_predicate =
- table_->NewInBloomFilterPredicate("value", allocator_slice,
included_bf_vec);
+ table_->NewInBloomFilterPredicate("value", included_bf_vec);
auto* included_predicate_clone1 = included_predicate->Clone();
auto* included_predicate_clone2 = included_predicate->Clone();
@@ -1434,7 +1431,7 @@ TEST_F(BloomFilterPredicateTest,
TestDirectBlockBloomFilterPredicate) {
{ Slice(reinterpret_cast<const uint8_t*>(excluded_bf.get()),
sizeof(*excluded_bf)) };
const size_t excluded_bf_vec_size = excluded_bf_vec.size();
auto* excluded_predicate =
- table_->NewInBloomFilterPredicate("value", allocator_slice,
excluded_bf_vec);
+ table_->NewInBloomFilterPredicate("value", excluded_bf_vec);
ASSERT_EQ(excluded_bf_vec_size, excluded_bf_vec.size());
int actual_count_excluded = CountRows(table_, { excluded_predicate });
diff --git a/src/kudu/client/scan_predicate-internal.h
b/src/kudu/client/scan_predicate-internal.h
index 543ae32..b3e8221 100644
--- a/src/kudu/client/scan_predicate-internal.h
+++ b/src/kudu/client/scan_predicate-internal.h
@@ -133,10 +133,8 @@ class InDirectBloomFilterPredicateData : public
KuduPredicate::Data {
public:
InDirectBloomFilterPredicateData(
ColumnSchema col,
- std::shared_ptr<BlockBloomFilterBufferAllocatorIf> allocator,
std::vector<DirectBlockBloomFilterUniqPtr> bloom_filters)
: col_(std::move(col)),
- allocator_(std::move(allocator)),
bloom_filters_(std::move(bloom_filters)) {
}
@@ -152,7 +150,6 @@ class InDirectBloomFilterPredicateData : public
KuduPredicate::Data {
// 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
// DirectBloomFilterDataDeleter to keep track of ownership.
- std::shared_ptr<BlockBloomFilterBufferAllocatorIf> allocator_;
std::vector<DirectBlockBloomFilterUniqPtr> bloom_filters_;
};
diff --git a/src/kudu/client/scan_predicate.cc
b/src/kudu/client/scan_predicate.cc
index fd4f352..af04418 100644
--- a/src/kudu/client/scan_predicate.cc
+++ b/src/kudu/client/scan_predicate.cc
@@ -195,25 +195,23 @@ Status
InDirectBloomFilterPredicateData::AddToScanSpec(ScanSpec* spec, Arena* /*
}
InDirectBloomFilterPredicateData* InDirectBloomFilterPredicateData::Clone()
const {
- // In the clone case, the objects are owned by
InDirectBloomFilterPredicateData
- // and hence use the default deleter with shared_ptr.
- shared_ptr<BlockBloomFilterBufferAllocatorIf> allocator_clone =
- CHECK_NOTNULL(allocator_->Clone());
-
vector<DirectBlockBloomFilterUniqPtr> bloom_filter_clones;
bloom_filter_clones.reserve(bloom_filters_.size());
+
+ // For the case of direct Bloom filter, we don't have access to the allocator
+ // so use the default allocator for cloning it.
+ auto* allocator = DefaultBlockBloomFilterBufferAllocator::GetSingleton();
for (const auto& bf : bloom_filters_) {
unique_ptr<BlockBloomFilter> bf_clone;
- CHECK_OK(bf->Clone(allocator_clone.get(), &bf_clone));
+ CHECK_OK(bf->Clone(allocator, &bf_clone));
- // Similarly for unique_ptr, specify that the BlockBloomFilter is owned by
+ // For unique_ptr, specify that the BlockBloomFilter is owned by
// InDirectBloomFilterPredicateData.
DirectBlockBloomFilterUniqPtr direct_bf_clone(
bf_clone.release(),
DirectBloomFilterDataDeleter<BlockBloomFilter>(true /*owned*/));
bloom_filter_clones.emplace_back(std::move(direct_bf_clone));
}
- return new InDirectBloomFilterPredicateData(col_, std::move(allocator_clone),
- std::move(bloom_filter_clones));
+ return new InDirectBloomFilterPredicateData(col_,
std::move(bloom_filter_clones));
}
KuduBloomFilter::KuduBloomFilter() {
