feat(cell_based_table): add sentinel column and cell_based muti_get #1590

wcy-fdu · 2022-04-05T14:46:45Z

What's changed and what's your intention?

This PR first adds sentinel column when serialize/deserialize, using None value for sentinel cell. While state_store is getting sentinel column, there are three cases:

return None: no exist row
return Some(value): exist row

Then cell_based muti_get is implemented by the design above.

BTW, some unit tests have been modified due to corresponding changes.

Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

codecov · 2022-04-05T15:09:55Z

Codecov Report

Merging #1590 (54f4959) into main (06bcfa9) will increase coverage by 0.00%.
The diff coverage is 95.01%.

@@           Coverage Diff           @@
##             main    #1590   +/-   ##
=======================================
  Coverage   71.37%   71.38%           
=======================================
  Files         600      600           
  Lines       77726    77721    -5     
=======================================
+ Hits        55477    55479    +2     
+ Misses      22249    22242    -7

Flag	Coverage Δ
rust	`71.38% <95.01%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/stream/src/executor_v2/mview/materialize.rs	`90.67% <ø> (ø)`
src/storage/src/cell_based_row_deserializer.rs	`98.09% <87.50%> (-0.96%)`	⬇️
.../stream/src/executor_v2/mview/table_state_tests.rs	`96.38% <94.41%> (+0.08%)`	⬆️
src/common/src/util/ordered/serde.rs	`92.82% <100.00%> (-0.19%)`	⬇️
src/storage/src/table/cell_based_table.rs	`77.13% <100.00%> (+1.75%)`	⬆️
...am/src/executor/managed_state/top_n/top_n_state.rs	`88.45% <100.00%> (ø)`
src/stream/src/executor_v2/mview/state.rs	`100.00% <100.00%> (ø)`
src/connector/src/filesystem/file_common.rs	`80.80% <0.00%> (+0.44%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

src/common/src/util/ordered/serde.rs

src/storage/src/cell_based_row_deserializer.rs

src/storage/src/table/cell_based_table.rs

src/common/src/util/ordered/serde.rs

src/storage/src/cell_based_row_deserializer.rs

src/storage/src/table/cell_based_table.rs

skyzh · 2022-04-07T07:53:46Z

src/storage/src/table/cell_based_table.rs

+            .concat();
+            let state_store_get_res = self.keyspace.get(&key.clone(), epoch).await?;
+            if let Some(state_store_get_res) = state_store_get_res {
+                get_res.push((key.clone(), state_store_get_res));


pls check all unnecessary clones

src/storage/src/table/cell_based_table.rs

lmatz · 2022-04-08T11:51:05Z

src/common/src/util/ordered/serde.rs

@@ -32,7 +32,7 @@ use crate::util::sort_util::{OrderPair, OrderType};
 use crate::util::value_encoding::serialize_cell;

 /// The special `cell_id` reserved for a whole null row is `i32::MIN`.
-pub const NULL_ROW_SPECIAL_CELL_ID: ColumnId = ColumnId::new(i32::MIN);
+pub const SENTINEL_CELL_ID: ColumnId = ColumnId::new(-1_i32);


IIRC, -1 is because we want SENTINEL_CELL_ID to be at the very beginning when we do scan? If so, may add some comments to it.
May give some examples in the comments, e.g. normal row, all-none row, what the storage format would be like.

Sounds good!

Not sure how we are encoding the column id now. If simply using be or le encoding, -1 might not be at the beginning when scanning.

lmatz · 2022-04-08T11:52:48Z

src/common/src/util/ordered/serde.rs

                result.push((key, None));
            }
        }
    }
-    if all_null {


May check whether the comments for this whole function is obsolete.

CLAassistant · 2022-04-08T14:52:17Z

All committers have signed the CLA.

skyzh · 2022-04-09T04:59:59Z

src/common/src/util/ordered/serde.rs

@@ -32,7 +32,7 @@ use crate::util::sort_util::{OrderPair, OrderType};
 use crate::util::value_encoding::serialize_cell;

 /// The special `cell_id` reserved for a whole null row is `i32::MIN`.
-pub const NULL_ROW_SPECIAL_CELL_ID: ColumnId = ColumnId::new(i32::MIN);
+pub const SENTINEL_CELL_ID: ColumnId = ColumnId::new(-1_i32);


Not sure how we are encoding the column id now. If simply using be or le encoding, -1 might not be at the beginning when scanning.

src/common/src/util/ordered/serde.rs

src/storage/src/cell_based_row_deserializer.rs

src/storage/src/table/cell_based_table.rs

skyzh · 2022-04-09T05:11:09Z

src/stream/src/executor/managed_state/top_n/top_n_state.rs

@@ -298,7 +298,7 @@ impl<S: StateStore, const TOP_N_TYPE: usize> ManagedTopNState<S, TOP_N_TYPE> {
        let pk_row_bytes = self
            .keyspace
            .scan_strip_prefix(
-                number_rows.map(|top_n_count| top_n_count * self.data_types.len()),
+                number_rows.map(|top_n_count| top_n_count * (self.data_types.len() + 1)),


So top-n will possibly scan more cells from storage? If there're nulls in storage, it will scan more that it needs. Hmm... better to have this refactored in the future. For example, use iterator-based APIs. (cc @lmatz)

Please also update the comments above

Yes, I talked to @wcy-fdu about this part requiring refactoring in the future.

skyzh · 2022-04-09T05:12:24Z

src/stream/src/executor_v2/mview/materialize.rs

@@ -235,7 +235,7 @@ mod tests {
                        .await
                        .unwrap()
                        .len(),
-                    6
+                    9


You can use get_row API to refactor this test case now.

also update comments above

src/stream/src/executor_v2/mview/state.rs

skyzh · 2022-04-09T05:13:51Z

src/stream/src/executor_v2/mview/table_state_tests.rs

+#[tokio::test]
+async fn test_get_row_by_muti_get() {
+    let state_store = MemoryStateStore::new();
+    let column_ids = vec![ColumnId::from(0), ColumnId::from(1), ColumnId::from(2)];


Can we also have some test cases like:

variable-length type? e.g. string

shuffled column id? e.g., column id 3, 2, 1 instead of 1, 2, 3

non-continuous ids? e.g., 1, 4, 8 instead of 1, 2, 3

Sounds good, I will add these cases and remove get_for_test in cell_based_table.

skyzh

LGTM. Better also have a test case like write using 1, 2, 3 column id, and read using 3, 2.

wcy-fdu · 2022-04-11T05:29:47Z

LGTM. Better also have a test case like write using 1, 2, 3 column id, and read using 3, 2.

Good idea.

BugenZhao · 2022-04-12T08:50:44Z

src/common/src/util/ordered/serde.rs

@@ -32,7 +32,7 @@ use crate::util::sort_util::{OrderPair, OrderType};
 use crate::util::value_encoding::serialize_cell;

 /// The special `cell_id` reserved for a whole null row is `i32::MIN`.


Please update this doc as well. :)

BugenZhao · 2022-04-12T08:52:11Z

src/storage/src/table/cell_based_table.rs

+                .map_err(err)?;
+            assert!(deserialize_res.is_none());
+        }
+        let pk_and_row = cell_based_row_deserializer.take();


It seems we can unwrap here?

BugenZhao · 2022-04-12T08:53:12Z

src/stream/src/executor_v2/mview/materialize.rs

                assert_eq!(
                    memory_state_store
                        .scan::<_, Vec<u8>>(.., None, u64::MAX)
                        .await
                        .unwrap()
                        .len(),
-                    6
+                    9
                );

                // FIXME(Bugen): restore this test by using new `RowTable` interface


Seems we can restore these tests with get_row.

draft: sentinel column

d123f9d

wcy-fdu marked this pull request as draft April 5, 2022 14:47

github-actions bot added the type/feature label Apr 5, 2022

remove rust

510c54b

add cell_based muti_get

871ec7a

wcy-fdu changed the title ~~feat: add sentinel column~~ feat(cell_based_table): add sentinel column and cell_based muti_get Apr 6, 2022

add cell_based muti_get

ef227a5

wcy-fdu marked this pull request as ready for review April 6, 2022 08:49

wcy-fdu requested a review from skyzh April 6, 2022 08:56

skyzh reviewed Apr 6, 2022

View reviewed changes

src/common/src/util/ordered/serde.rs Outdated Show resolved Hide resolved

src/storage/src/cell_based_row_deserializer.rs Outdated Show resolved Hide resolved

src/storage/src/table/cell_based_table.rs Outdated Show resolved Hide resolved

skyzh reviewed Apr 6, 2022

View reviewed changes

src/storage/src/table/cell_based_table.rs Outdated Show resolved Hide resolved

wcy-fdu added 2 commits April 6, 2022 18:05

fix

3f3b9e3

remove all_none

e0cf5d2

skyzh reviewed Apr 7, 2022

View reviewed changes

wcy-fdu added 3 commits April 8, 2022 16:57

remove all_none

4fd9729

add comments

c662040

resolve conflict

0194439

wcy-fdu requested a review from lmatz April 8, 2022 10:06

lmatz reviewed Apr 8, 2022

View reviewed changes

skyzh reviewed Apr 9, 2022

View reviewed changes

add more cases in cell_based unit tests

07a8178

skyzh approved these changes Apr 11, 2022

View reviewed changes

wcy-fdu and others added 2 commits April 11, 2022 13:36

add some comments

c7ed6c7

Merge branch 'main' into wcy_add_sentinel_column

54f4959

wcy-fdu merged commit 0bfc42c into main Apr 11, 2022

wcy-fdu deleted the wcy_add_sentinel_column branch April 11, 2022 06:07

BugenZhao reviewed Apr 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cell_based_table): add sentinel column and cell_based muti_get #1590

feat(cell_based_table): add sentinel column and cell_based muti_get #1590

wcy-fdu commented Apr 5, 2022 •

edited

codecov bot commented Apr 5, 2022 •

edited

skyzh Apr 7, 2022

lmatz Apr 8, 2022

wcy-fdu Apr 8, 2022

skyzh Apr 9, 2022

lmatz Apr 8, 2022

CLAassistant commented Apr 8, 2022 •

edited

skyzh Apr 9, 2022

skyzh Apr 9, 2022

skyzh Apr 9, 2022

lmatz Apr 9, 2022

skyzh Apr 9, 2022

skyzh Apr 9, 2022

skyzh Apr 9, 2022

wcy-fdu Apr 11, 2022

skyzh left a comment •

edited

wcy-fdu commented Apr 11, 2022

BugenZhao Apr 12, 2022

BugenZhao Apr 12, 2022

BugenZhao Apr 12, 2022

		@@ -32,7 +32,7 @@ use crate::util::sort_util::{OrderPair, OrderType};
		use crate::util::value_encoding::serialize_cell;

		/// The special `cell_id` reserved for a whole null row is `i32::MIN`.

feat(cell_based_table): add sentinel column and cell_based muti_get #1590

feat(cell_based_table): add sentinel column and cell_based muti_get #1590

Conversation

wcy-fdu commented Apr 5, 2022 • edited

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Apr 5, 2022 • edited

Codecov Report

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

CLAassistant commented Apr 8, 2022 • edited

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 left a comment • edited

Choose a reason for hiding this comment

wcy-fdu commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcy-fdu commented Apr 5, 2022 •

edited

codecov bot commented Apr 5, 2022 •

edited

CLAassistant commented Apr 8, 2022 •

edited

skyzh left a comment •

edited