feat(storage): record first_key in block_index_builder #598

adlternative · 2022-04-03T04:00:49Z

This patch want to complete #589 first step, which record first_key in order to support seek.

Signed-off-by: ZheNing Hu adlternative@gmail.com

skyzh

By the way, first_key is only useful when the column is fully sorted. So we can have a parameter on column builder whether to store the first_key or not.

skyzh · 2022-04-03T05:37:03Z

src/storage/secondary/block/block_index_builder.rs

@@ -83,4 +87,8 @@ impl BlockIndexBuilder {
    pub fn into_index(self) -> Vec<BlockIndex> {
        self.indexes
    }
+
+    pub fn set_first_key(&mut self, first_key: Vec<u8>) {


I'd prefer having the signature as

pub fn set_first_key(&mut self, first_key: &[u8]) { self.first_key.clear(); self.first_key.extend(first_key); }

So as to reduce allocation as much as possible.

By the way, it seems that first_key will only be used on finish_block. Is it possible to make it a parameter of finish_block instead of having a separate function?

By the way, first_key is only useful when the column is fully sorted. So we can have a parameter on column builder whether to store the first_key or not.

Agree. But I am curious about where can we know that column need sort or not? Maybe pass from ColumnBuilderOptions later?

I'd prefer having the signature as

pub fn set_first_key(&mut self, first_key: &[u8]) { self.first_key.clear(); self.first_key.extend(first_key); }

So as to reduce allocation as much as possible.

By the way, it seems that first_key will only be used on finish_block. Is it possible to make it a parameter of finish_block instead of having a separate function?

This means that we need to modify PrimitiveColumnBuilder<T>::finish_builder() interface, that's ok...

skyzh · 2022-04-03T05:39:20Z

src/storage/secondary/column/primitive_column_builder.rs

@@ -121,7 +121,7 @@ pub fn append_one_by_one<'a, A: Array>(
 impl<T: PrimitiveFixedWidthEncode> ColumnBuilder<T::ArrayType> for PrimitiveColumnBuilder<T> {
    fn append(&mut self, array: &T::ArrayType) {
        let mut iter = array.iter().peekable();
-
+        let mut first_flag = true;


first_flag should be an inner state of ColumnBuilder instead of a local variable. One append doesn't necessarily produce a block.

I think a better way is to store the first_key in the struct on switching builder (when self.current_builder.is_none())?

first_flag should be an inner state of ColumnBuilder instead of a local variable. One append doesn't necessarily produce a block.

I think a better way is to store the first_key in the struct on switching builder (when self.current_builder.is_none())?

Yes, the condition self.current_builder.is_none() can be reused.

adlternative · 2022-04-04T07:21:11Z

By the way, should we apply this feature to CharColumnBuilder or BlobColumnBuilder, they have similar interface...

skyzh · 2022-04-04T07:23:29Z

By the way, should we apply this feature to CharColumnBuilder or BlobColumnBuilder, they have similar interface...

Yes, indeed. But we can work on primitive types first, and add support for other columns later.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>

adlternative · 2022-04-06T02:07:38Z

Hi, now we can use a new option record_first_key to control if record first key of the block, is this change reasonable?
I am not sure about if named it as is_sorted or something else will be better?
@skyzh

skyzh · 2022-04-06T02:24:54Z

Hi, now we can use a new option record_first_key to control if record first key of the block, is this change reasonable?
I am not sure about if named it as is_sorted or something else will be better?

I think record_first_key is clear enough! I'll take a look at this PR soon. Thanks!

skyzh

Generally LGTM, but there're still some misc fixes to do. Thanks for your contribution!

skyzh · 2022-04-06T04:38:39Z

src/storage/secondary/block/block_index_builder.rs

    ) {
        self.indexes.push(BlockIndex {
            offset: column_data.len() as u64,
            length: block_data.len() as u64 + BLOCK_HEADER_SIZE as u64,
            first_rowid: self.last_row_count as u32,
            row_count: (self.row_count - self.last_row_count) as u32,
            /// TODO(chi): support sort key
-            first_key: "".into(),
+            first_key: first_key.drain(..).collect(),


I'd prefer to get a full ownership of first_key in this function. Otherwise this interface would look confusing 🤣

skyzh · 2022-04-06T04:40:11Z

src/storage/secondary/column/blob_column_builder.rs

+            &mut self.data,
+            &mut block_data,
+            stats,
+            &mut self.first_key,


Let's drain here: self.first_key.drain(..).collect_vec()

skyzh · 2022-04-06T04:40:41Z

src/storage/secondary/column/blob_column_builder.rs

@@ -80,6 +89,11 @@ impl ColumnBuilder<BlobArray> for BlobColumnBuilder {
                        PlainBlobBlockBuilder::new(self.options.target_block_size - 16),
                    ));
                }
+                if let Some(to_be_appended) = iter.peek() {
+                    if self.options.record_first_key && to_be_appended.is_some() {


None is also a valid first_key. I think we can decide on encoding of None keys.

Maybe we can use \0 to represent null keys, and \1 + BlobEncode to represent non-null keys?

Also for the encoding of the key, we can use utilities provided in https://github.com/risinglightdb/risinglight/blob/main/src/storage/secondary/encode.rs

For blob types, simply call to_be_appended.to_byte_slice().to_vec(), and store it in first_key.

We can also assert!(self.first_key.is_empty()) here, for an extra layer of sanity check.

None is also a valid first_key. I think we can decide on encoding of None keys.

Maybe we can use \0 to represent null keys, and \1 + BlobEncode to represent non-null keys?

So we use \1 for CharColumnBuilder too, right?

Yes, we use this as a temporary encoding for nullable elements, for both char, blob and primitive types.

Or we can have two fields: is_first_key_null and first_key in proto. Maybe this could be more intuitive? If first_key is null, first_key field is set to empty, and is_first_key_null is set to true?

skyzh · 2022-04-06T04:47:36Z

By the way, please have some basic unit tests, at least for primitive type builders 🥰

adlternative · 2022-04-08T12:03:03Z

By the way, please have some basic unit tests, at least for primitive type builders smiling_face_with_three_hearts

Hi, I have followed your suggestions to add is_first_key_null to proto and add some tests for primitive type builders. 😉
@skyzh

skyzh

Still some small issues. Thanks again for your contribution!

skyzh · 2022-04-08T12:13:00Z

src/storage/secondary/column/char_column_builder.rs

+                    if self.options.record_first_key && to_be_appended.is_some() {
+                        self.is_first_key_null = false;
+                        self.first_key = to_be_appended.unwrap().as_bytes().to_vec();
+                    }


It seems that once is_first_key_null is set to false, it will never be set back to true?

We can define self.first_key as Option<Vec<u8>>, so as to make the logic clearer.

if self.options.record_first_key { self.first_key = to_be_appended.map(|x| x.as_bytes().to_vec()); }

And we can modify finish_block to accept first_key: Option<Vec<u8>>. Inside the block builder, we match first_key. If null, set is_first_key_null to true. Otherwise, set first_key to the inner value.

Still some small issues. Thanks again for your contribution!

Thanks also for your review too! 😭

It seems that once is_first_key_null is set to false, it will never be set back to true?

We can define self.first_key as Option<Vec<u8>>, so as to make the logic clearer.

if self.options.record_first_key { self.first_key = to_be_appended.map(|x| x.as_bytes().to_vec()); }

And we can modify finish_block to accept first_key: Option<Vec<u8>>. Inside the block builder, we match first_key. If null, set is_first_key_null to true. Otherwise, set first_key to the inner value.

Maybe finish_block() will be like this?

pub fn finish_block( &mut self, block_type: BlockType, column_data: &mut Vec<u8>, block_data: &mut Vec<u8>, stats: Vec<BlockStatistics>, first_key: &mut Option<Vec<u8>>, ) { self.indexes.push(BlockIndex { offset: column_data.len() as u64, length: block_data.len() as u64 + BLOCK_HEADER_SIZE as u64, first_rowid: self.last_row_count as u32, row_count: (self.row_count - self.last_row_count) as u32, /// TODO(chi): support sort key first_key: match first_key { Some(x) => x.drain(..).collect(), None => Default::default(), }, stats, is_first_key_null: first_key.is_none(), }); ...

Exactly! And I’d prefer to have Option<Vec<u8>> as parameter instead of &mut. As block is typically large, this re-allocation is acceptable.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>

skyzh

LGTM, good work!

adlternative · 2022-04-09T06:22:36Z

LGTM, good work!

Thanks a lot! @skyzh

adlternative force-pushed the record_first_key branch from aa51882 to bf661d9 Compare April 3, 2022 04:46

skyzh reviewed Apr 3, 2022

View reviewed changes

feat(storage): record first_key in block_index_builder

5301767

Signed-off-by: ZheNing Hu <adlternative@gmail.com>

adlternative force-pushed the record_first_key branch from bf661d9 to 5301767 Compare April 4, 2022 08:07

skyzh reviewed Apr 6, 2022

View reviewed changes

adlternative force-pushed the record_first_key branch 2 times, most recently from ef44350 to 107f9da Compare April 8, 2022 12:02

skyzh reviewed Apr 8, 2022

View reviewed changes

feat(storage): add record_first_key option to ColumnBuilderOptions

8bcc46e

Signed-off-by: ZheNing Hu <adlternative@gmail.com>

adlternative force-pushed the record_first_key branch from 107f9da to 8bcc46e Compare April 9, 2022 03:56

skyzh approved these changes Apr 9, 2022

View reviewed changes

skyzh enabled auto-merge (squash) April 9, 2022 04:09

Merge branch 'main' into record_first_key

5df6eca

skyzh merged commit f591970 into risinglightdb:main Apr 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): record first_key in block_index_builder #598

feat(storage): record first_key in block_index_builder #598

adlternative commented Apr 3, 2022

skyzh left a comment

skyzh Apr 3, 2022

adlternative Apr 4, 2022

adlternative Apr 4, 2022

skyzh Apr 3, 2022

adlternative Apr 4, 2022

adlternative commented Apr 4, 2022

skyzh commented Apr 4, 2022

adlternative commented Apr 6, 2022 •

edited

skyzh commented Apr 6, 2022

skyzh left a comment

skyzh Apr 6, 2022

skyzh Apr 6, 2022

skyzh Apr 6, 2022

skyzh Apr 6, 2022

skyzh Apr 6, 2022

adlternative Apr 6, 2022

skyzh Apr 6, 2022

skyzh Apr 6, 2022 •

edited

skyzh commented Apr 6, 2022

adlternative commented Apr 8, 2022

skyzh left a comment

skyzh Apr 8, 2022

adlternative Apr 8, 2022

adlternative Apr 8, 2022

skyzh Apr 8, 2022 •

edited

skyzh left a comment

adlternative commented Apr 9, 2022

feat(storage): record first_key in block_index_builder #598

feat(storage): record first_key in block_index_builder #598

Conversation

adlternative commented Apr 3, 2022

skyzh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlternative commented Apr 4, 2022

skyzh commented Apr 4, 2022

adlternative commented Apr 6, 2022 • edited

skyzh commented Apr 6, 2022

skyzh 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

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

skyzh Apr 6, 2022 • edited

Choose a reason for hiding this comment

skyzh commented Apr 6, 2022

adlternative commented Apr 8, 2022

skyzh 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

skyzh Apr 8, 2022 • edited

Choose a reason for hiding this comment

skyzh left a comment

Choose a reason for hiding this comment

adlternative commented Apr 9, 2022

adlternative commented Apr 6, 2022 •

edited

skyzh Apr 6, 2022 •

edited

skyzh Apr 8, 2022 •

edited