Builders for partition filter #1952

maysamyabandeh · 2017-03-06T19:16:35Z

This is the second split of this pull request: #1891 which includes only the builder part. The testing will be included in the third split, where the reader is also included.

facebook-github-bot · 2017-03-06T19:17:16Z

@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug · 2017-03-07T02:13:19Z

table/filter_block.h

+  Slice Finish() {                             // Generate Filter
+    const BlockHandle empty_handle;
+    Status dont_care_status;
+    return Finish(empty_handle, &dont_care_status);


assert(status.ok()) ? Just to avoid someone misuse it..

sure. good thinking.

facebook-github-bot · 2017-03-07T04:23:38Z

@maysamyabandeh updated the pull request - view changes - changes since last import

yiwu-arbug

Main flow look good to me. Some comments.

yiwu-arbug · 2017-03-07T02:19:14Z

table/full_filter_block.h

@@ -96,14 +99,13 @@ class FullFilterBlockReader : public FilterBlockReader {

 private:
  const SliceTransform* prefix_extractor_;
+  Slice contents_;
+  bool MayMatch(const Slice& entry);


nit: order methods after members, as before?

yiwu-arbug · 2017-03-07T02:31:35Z

table/index_builder.cc

@@ -24,21 +25,22 @@ namespace rocksdb {
 IndexBuilder* IndexBuilder::CreateIndexBuilder(
    BlockBasedTableOptions::IndexType index_type,
    const InternalKeyComparator* comparator,
+    const SliceTransform* slice_transform,


What's the difference between slice_transform and prefix_extractor?

prefix_extractor comes from _ioptions.prefix_extractor which is of type SliceTransform.

slice_transform comes from this->internal_prefix_transform which is of type InternalKeySliceTransform initialized by prefix_extractor's transform member. slice_transform is used only by the hash index builder.

previously I was reusing InternalKeySliceTransform for also partitioned filter builder which turns out to be wrong. So here we distinguish between the two, use slice_transform for hash (as it was before) and use _ioptions.prefix_extractor for partitioned filter.

I understand the naming is confusing here. I am going to ahead doing this renaming:
slice_transform => int_key_slice_transform
and also use the explicit type of InternalKeySliceTransform for it.

Actually I realized that after separating index and filter builders prefix_extractor is not need anymore, so i am going to remove it too.

yiwu-arbug · 2017-03-07T04:43:39Z

table/index_builder.cc

-                                       index_block_restart_interval);
+      return PartitionedIndexBuilder::CreateIndexBuilder(
+          comparator, prefix_extractor, index_block_restart_interval,
+          index_per_partition, table_opt);


table_opt should already have index_block_restart_interval and index_per_partition?

yeah i had to add table_opt later in the coding. yeah it makes sense to remove the two parameters.

yiwu-arbug · 2017-03-07T04:45:18Z

table/index_builder.cc

+    virtual void AddKey(const Slice& key) { assert(0); }
+    virtual Slice Finish(std::unique_ptr<const char[]>* buf) { assert(0); }
+  };
+  FilterBitsBuilder* filter_bits_builder =


where this is being use?

good catch. it was needed when index and filter builders were mixed. but not anymore.

yiwu-arbug · 2017-03-07T04:46:42Z

table/index_builder.h

+  bool cut_filter_block =
+      false;  // true if it should cut the next filter partition block
+};
+}


// namespace rocksdb

why is that "make format" does not report this issues?

make format probably have different set of lint rules than what phabricator have..

yiwu-arbug · 2017-03-07T05:36:33Z

table/partitioned_filter_block.cc

-  if (UNLIKELY(entries_.empty())) {
-    index_blocks->index_block_contents = index_block_builder_.Finish();
-    return Status::OK();
+  MayBeCutAFilterBlock();


seems you shouldn't call MayBeCutAFitlerBlock if finishing_filters=true?

when finishing_filters=true then MayBeCutAFitlerBlock will be essentially noop. but It would good to make it clear in the code.

yiwu-arbug · 2017-03-07T05:36:55Z

table/partitioned_filter_block.h

      false;  // true if Finish is called once but not complete yet.
+  // The policy of when cut a filter block and Finish it
+  void MayBeCutAFilterBlock();


nit: s/MayBe/Maybe/

yiwu-arbug · 2017-03-07T05:41:20Z

table/block_based_table_builder.cc

+// Create a filter block builder based on its type.
+FilterBlockBuilder* CreateFilterBlockBuilder(
+    const ImmutableCFOptions& opt, const BlockBasedTableOptions& table_opt,
+    PartitionedIndexBuilder* const part_index_builder) {


I would prefer s/part_index_builder/p_index_builder/

yiwu-arbug · 2017-03-07T05:42:43Z

table/block_based_table_builder.cc

+          table_options.index_per_partition, table_options));
+    }
+    if (skip_filters) {
+      filter_block = nullptr;


s/filter_block/filter_builder/ ?

oh yeah you said before. sure.

yiwu-arbug · 2017-03-07T05:44:47Z

table/block_based_table_builder.cc

@@ -374,6 +399,8 @@ void BlockBasedTableBuilder::Add(const Slice& key, const Slice& value) {
      }
    }

+    // Note: PartitionedFilterBlockBuilder requires adding to filter_block to be


... requires key being added to filter builder after being added to index builder.

facebook-github-bot · 2017-03-07T18:38:25Z

@maysamyabandeh updated the pull request - view changes - changes since last import

yiwu-arbug · 2017-03-07T18:46:06Z

table/partitioned_filter_block.cc

@@ -13,27 +13,27 @@ namespace rocksdb {
 PartitionedFilterBlockBuilder::PartitionedFilterBlockBuilder(
    const SliceTransform* prefix_extractor, bool whole_key_filtering,
    FilterBitsBuilder* filter_bits_builder, int index_block_restart_interval,
-    PartitionedIndexBuilder* const part_index_builder)
+    PartitionedIndexBuilder* const p_index_builder)


This is a constant pointer. Are you wanting to have a pointer to constant (e.g. const T*) ?

No I meant a constant pointer as there is no reason for the pointer to be changed.

facebook-github-bot · 2017-03-07T19:20:13Z

@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug · 2017-03-07T19:35:20Z

table/partitioned_filter_block.cc

-    Entry& last_entry = entries_.front();
+Slice PartitionedFilterBlockBuilder::Finish(
+    const BlockHandle& last_partition_block_handle, Status* status) {
+  if (finishing_filters == true) {


It took me some time to verify the logic of this method. Would this be a little bit more clear?

if (filter.empty() && !finishing_filters) { return Slice(); } else if (!filter.empty()) { auto filter = filters.front(); filters.pop_front(); finish_filters = true; index_builder.Add(filter.key); return filter.filter; } else { return index_builder.Finish(); }

yiwu-arbug

Looking good to me!

This is the second split of this pull request: facebook#1891 which includes only the builder part. The testing will be included in the third split, where the reader is also included.

facebook-github-bot · 2017-03-07T20:31:26Z

@maysamyabandeh updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-03-07T20:32:26Z

@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

maysamyabandeh requested a review from yiwu-arbug March 6, 2017 19:16

facebook-github-bot added the CLA Signed label Mar 6, 2017

yiwu-arbug reviewed Mar 7, 2017

View reviewed changes

yiwu-arbug approved these changes Mar 7, 2017

View reviewed changes

Maysam Yabandeh added 5 commits March 7, 2017 12:29

Partitioned Filters, the builder part

d3b7002

This is the second split of this pull request: facebook#1891 which includes only the builder part. The testing will be included in the third split, where the reader is also included.

apply comments

393a59c

minor

a953e04

apply comments

4aeca11

add comments to PartitionedFilterBlockBuilder::Finish

69b608a

maysamyabandeh force-pushed the builders-for-partition-filter branch from a092fee to 69b608a Compare March 7, 2017 20:30

facebook-github-bot closed this in 54b4341 Mar 7, 2017

Builders for partition filter #1952

Builders for partition filter #1952

Conversation

maysamyabandeh commented Mar 6, 2017 • edited Loading

facebook-github-bot commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 7, 2017

yiwu-arbug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysamyabandeh Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 7, 2017

Choose a reason for hiding this comment

yiwu-arbug left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 7, 2017

facebook-github-bot commented Mar 7, 2017

maysamyabandeh commented Mar 6, 2017 •

edited

Loading

maysamyabandeh Mar 7, 2017 •

edited

Loading