Skip to content

Commit

Permalink
Improve const correctness in BlockBasedTableReader (facebook#5383)
Browse files Browse the repository at this point in the history 
Summary:
Many methods are passing around pointers to non-const objects when in fact
they do not/should not modify said objects. The patch makes the semantics
clearer and also helps from a thread safety point-of-view by changing some
pointers to pointers-to-const and marking some instance methods as const.
Pull Request resolved: facebook#5383

Differential Revision: D15562770

Pulled By: ltamasi

fbshipit-source-id: 89361dadbb8b25bbe54d17e8da28fee24a2419af
  • Loading branch information
@ltamasi @facebook-github-bot
ltamasi authored and facebook-github-bot committed May 31, 2019
1 parent cb094e1 commit a3609b7
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 33 deletions.
36 changes: 19 additions & 17 deletions table/block_based/block_based_table_reader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,19 +203,20 @@ bool PrefixExtractorChanged(const TableProperties* table_properties,
// in the cache or not.
class BlockBasedTable::IndexReaderCommon : public BlockBasedTable::IndexReader {
public:
IndexReaderCommon(BlockBasedTable* t, CachableEntry<Block>&& index_block)
IndexReaderCommon(const BlockBasedTable* t,
CachableEntry<Block>&& index_block)
: table_(t), index_block_(std::move(index_block)) {
assert(table_ != nullptr);
}

protected:
static Status ReadIndexBlock(BlockBasedTable* table,
static Status ReadIndexBlock(const BlockBasedTable* table,
FilePrefetchBuffer* prefetch_buffer,
const ReadOptions& read_options,
GetContext* get_context,
CachableEntry<Block>* index_block);

BlockBasedTable* table() const { return table_; }
const BlockBasedTable* table() const { return table_; }

const InternalKeyComparator* internal_comparator() const {
assert(table_ != nullptr);
Expand Down Expand Up @@ -256,12 +257,12 @@ class BlockBasedTable::IndexReaderCommon : public BlockBasedTable::IndexReader {
}

private:
BlockBasedTable* table_;
const BlockBasedTable* table_;
CachableEntry<Block> index_block_;
};

Status BlockBasedTable::IndexReaderCommon::ReadIndexBlock(
BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer,
const BlockBasedTable* table, FilePrefetchBuffer* prefetch_buffer,
const ReadOptions& read_options, GetContext* get_context,
CachableEntry<Block>* index_block) {
PERF_TIMER_GUARD(read_index_block_nanos);
Expand Down Expand Up @@ -304,7 +305,7 @@ class PartitionIndexReader : public BlockBasedTable::IndexReaderCommon {
// `PartitionIndexReader`.
// On success, index_reader will be populated; otherwise it will remain
// unmodified.
static Status Create(BlockBasedTable* table,
static Status Create(const BlockBasedTable* table,
FilePrefetchBuffer* prefetch_buffer, bool use_cache,
bool prefetch, bool pin, IndexReader** index_reader) {
assert(table != nullptr);
Expand Down Expand Up @@ -473,7 +474,8 @@ class PartitionIndexReader : public BlockBasedTable::IndexReaderCommon {
}

private:
PartitionIndexReader(BlockBasedTable* t, CachableEntry<Block>&& index_block)
PartitionIndexReader(const BlockBasedTable* t,
CachableEntry<Block>&& index_block)
: IndexReaderCommon(t, std::move(index_block)) {}

std::unordered_map<uint64_t, CachableEntry<Block>> partition_map_;
Expand All @@ -488,7 +490,7 @@ class BinarySearchIndexReader : public BlockBasedTable::IndexReaderCommon {
// `BinarySearchIndexReader`.
// On success, index_reader will be populated; otherwise it will remain
// unmodified.
static Status Create(BlockBasedTable* table,
static Status Create(const BlockBasedTable* table,
FilePrefetchBuffer* prefetch_buffer, bool use_cache,
bool prefetch, bool pin, IndexReader** index_reader) {
assert(table != nullptr);
Expand Down Expand Up @@ -553,7 +555,7 @@ class BinarySearchIndexReader : public BlockBasedTable::IndexReaderCommon {
}

private:
BinarySearchIndexReader(BlockBasedTable* t,
BinarySearchIndexReader(const BlockBasedTable* t,
CachableEntry<Block>&& index_block)
: IndexReaderCommon(t, std::move(index_block)) {}
};
Expand All @@ -562,7 +564,7 @@ class BinarySearchIndexReader : public BlockBasedTable::IndexReaderCommon {
// key.
class HashIndexReader : public BlockBasedTable::IndexReaderCommon {
public:
static Status Create(BlockBasedTable* table,
static Status Create(const BlockBasedTable* table,
FilePrefetchBuffer* prefetch_buffer,
InternalIterator* meta_index_iter, bool use_cache,
bool prefetch, bool pin, IndexReader** index_reader) {
Expand Down Expand Up @@ -699,7 +701,7 @@ class HashIndexReader : public BlockBasedTable::IndexReaderCommon {
}

private:
HashIndexReader(BlockBasedTable* t, CachableEntry<Block>&& index_block)
HashIndexReader(const BlockBasedTable* t, CachableEntry<Block>&& index_block)
: IndexReaderCommon(t, std::move(index_block)) {}

std::unique_ptr<BlockPrefixIndex> prefix_index_;
Expand Down Expand Up @@ -1188,7 +1190,7 @@ Status BlockBasedTable::ReadRangeDelBlock(
}

Status BlockBasedTable::ReadCompressionDictBlock(
Rep* rep, FilePrefetchBuffer* prefetch_buffer,
const Rep* rep, FilePrefetchBuffer* prefetch_buffer,
std::unique_ptr<const BlockContents>* compression_dict_block) {
assert(compression_dict_block != nullptr);
Status s;
Expand Down Expand Up @@ -1842,7 +1844,7 @@ CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
}

CachableEntry<UncompressionDict>
BlockBasedTable::GetUncompressionDict(Rep* rep,
BlockBasedTable::GetUncompressionDict(const Rep* rep,
FilePrefetchBuffer* prefetch_buffer,
bool no_io, GetContext* get_context) {
if (!rep->table_options.cache_index_and_filter_blocks) {
Expand Down Expand Up @@ -1925,7 +1927,7 @@ BlockBasedTable::GetUncompressionDict(Rep* rep,
// differs from the one in mutable_cf_options and index type is HashBasedIndex
InternalIteratorBase<BlockHandle>* BlockBasedTable::NewIndexIterator(
const ReadOptions& read_options, bool disable_prefix_seek,
IndexBlockIter* input_iter, GetContext* get_context) {
IndexBlockIter* input_iter, GetContext* get_context) const {
assert(rep_ != nullptr);
assert(rep_->index_reader != nullptr);

Expand All @@ -1941,7 +1943,7 @@ InternalIteratorBase<BlockHandle>* BlockBasedTable::NewIndexIterator(
// If input_iter is not null, update this iter and return it
template <typename TBlockIter>
TBlockIter* BlockBasedTable::NewDataBlockIterator(
Rep* rep, const ReadOptions& ro, const BlockHandle& handle,
const Rep* rep, const ReadOptions& ro, const BlockHandle& handle,
TBlockIter* input_iter, bool is_index, bool key_includes_seq,
bool index_key_is_full, GetContext* get_context, Status s,
FilePrefetchBuffer* prefetch_buffer) {
Expand Down Expand Up @@ -2164,7 +2166,7 @@ Status BlockBasedTable::RetrieveBlock(
}

BlockBasedTable::PartitionedIndexIteratorState::PartitionedIndexIteratorState(
BlockBasedTable* table,
const BlockBasedTable* table,
std::unordered_map<uint64_t, CachableEntry<Block>>* block_map,
bool index_key_includes_seq, bool index_key_is_full)
: table_(table),
Expand Down Expand Up @@ -2214,7 +2216,7 @@ BlockBasedTable::PartitionedIndexIteratorState::NewSecondaryIterator(
bool BlockBasedTable::PrefixMayMatch(
const Slice& internal_key, const ReadOptions& read_options,
const SliceTransform* options_prefix_extractor,
const bool need_upper_bound_check) {
const bool need_upper_bound_check) const {
if (!rep_->filter_policy) {
return true;
}
Expand Down
26 changes: 10 additions & 16 deletions table/block_based/block_based_table_reader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class BlockBasedTable : public TableReader {
bool PrefixMayMatch(const Slice& internal_key,
const ReadOptions& read_options,
const SliceTransform* options_prefix_extractor,
const bool need_upper_bound_check);
const bool need_upper_bound_check) const;

// Returns a new iterator over the table contents.
// The result of NewIterator() is initially invalid (caller must
Expand Down Expand Up @@ -215,18 +215,12 @@ class BlockBasedTable : public TableReader {
struct Rep;

Rep* get_rep() { return rep_; }
const Rep* get_rep() const { return rep_; }

// input_iter: if it is not null, update this one and return it as Iterator
template <typename TBlockIter>
static TBlockIter* NewDataBlockIterator(
Rep* rep, const ReadOptions& ro, const Slice& index_value,
TBlockIter* input_iter = nullptr, bool is_index = false,
bool key_includes_seq = true, bool index_key_is_full = true,
GetContext* get_context = nullptr,
FilePrefetchBuffer* prefetch_buffer = nullptr);
template <typename TBlockIter>
static TBlockIter* NewDataBlockIterator(
Rep* rep, const ReadOptions& ro, const BlockHandle& block_hanlde,
const Rep* rep, const ReadOptions& ro, const BlockHandle& block_hanlde,
TBlockIter* input_iter = nullptr, bool is_index = false,
bool key_includes_seq = true, bool index_key_is_full = true,
GetContext* get_context = nullptr, Status s = Status(),
Expand Down Expand Up @@ -283,7 +277,7 @@ class BlockBasedTable : public TableReader {
const SliceTransform* prefix_extractor = nullptr) const;

static CachableEntry<UncompressionDict> GetUncompressionDict(
Rep* rep, FilePrefetchBuffer* prefetch_buffer, bool no_io,
const Rep* rep, FilePrefetchBuffer* prefetch_buffer, bool no_io,
GetContext* get_context);

// Get the iterator from the index reader.
Expand All @@ -299,7 +293,7 @@ class BlockBasedTable : public TableReader {
InternalIteratorBase<BlockHandle>* NewIndexIterator(
const ReadOptions& read_options, bool need_upper_bound_check = false,
IndexBlockIter* input_iter = nullptr,
GetContext* get_context = nullptr);
GetContext* get_context = nullptr) const;

// Read block cache from block caches (if set): block_cache and
// block_cache_compressed.
Expand Down Expand Up @@ -386,7 +380,7 @@ class BlockBasedTable : public TableReader {
InternalIterator* meta_iter,
const InternalKeyComparator& internal_comparator);
static Status ReadCompressionDictBlock(
Rep* rep, FilePrefetchBuffer* prefetch_buffer,
const Rep* rep, FilePrefetchBuffer* prefetch_buffer,
std::unique_ptr<const BlockContents>* compression_dict_block);
static Status PrefetchIndexAndFilterBlocks(
Rep* rep, FilePrefetchBuffer* prefetch_buffer,
Expand Down Expand Up @@ -430,15 +424,15 @@ class BlockBasedTable::PartitionedIndexIteratorState
: public TwoLevelIteratorState {
public:
PartitionedIndexIteratorState(
BlockBasedTable* table,
const BlockBasedTable* table,
std::unordered_map<uint64_t, CachableEntry<Block>>* block_map,
const bool index_key_includes_seq, const bool index_key_is_full);
InternalIteratorBase<BlockHandle>* NewSecondaryIterator(
const BlockHandle& index_value) override;

private:
// Don't own table_
BlockBasedTable* table_;
const BlockBasedTable* table_;
std::unordered_map<uint64_t, CachableEntry<Block>>* block_map_;
bool index_key_includes_seq_;
bool index_key_is_full_;
Expand Down Expand Up @@ -561,7 +555,7 @@ struct BlockBasedTable::Rep {
template <class TBlockIter, typename TValue = Slice>
class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
public:
BlockBasedTableIterator(BlockBasedTable* table,
BlockBasedTableIterator(const BlockBasedTable* table,
const ReadOptions& read_options,
const InternalKeyComparator& icomp,
InternalIteratorBase<BlockHandle>* index_iter,
Expand Down Expand Up @@ -681,7 +675,7 @@ class BlockBasedTableIterator : public InternalIteratorBase<TValue> {
void CheckOutOfBound();

private:
BlockBasedTable* table_;
const BlockBasedTable* table_;
const ReadOptions read_options_;
const InternalKeyComparator& icomp_;
UserComparatorWrapper user_comparator_;
Expand Down

0 comments on commit a3609b7

Please sign in to comment.