feat(storage): replace MViewTable with CellBasedTable and add relational interfaces #1046

wcy-fdu · 2022-03-17T12:57:21Z

What's changed and what's your intention?

This is a draft of cell-based table, which first replaces(rename) MViewTable with CellBasedTable, because MViewTable is already cell-based.

Then this PR adds the relational interfaces for CellBasedTable, such as insert_row, update_row, delete_row , get_row and batch_insert_row. And its scan interface is the same as previous MViewTableIter.

There are some details to be updated, such as the starting key of scan.

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)

Tracking issue #849

codecov · 2022-03-17T13:14:15Z

Codecov Report

Merging #1046 (b4615aa) into main (b54424d) will decrease coverage by 0.07%.
The diff coverage is 55.01%.

@@             Coverage Diff              @@
##               main    #1046      +/-   ##
============================================
- Coverage     70.28%   70.20%   -0.08%     
  Complexity     2766     2766              
============================================
  Files           997      997              
  Lines         83966    84087     +121     
  Branches       1790     1790              
============================================
+ Hits          59016    59034      +18     
- Misses        24059    24162     +103     
  Partials        891      891

Flag	Coverage Δ
java	`61.01% <ø> (ø)`
rust	`72.34% <55.01%> (-0.11%)`	⬇️

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

Impacted Files	Coverage Δ
rust/storage/src/lib.rs	`100.00% <ø> (ø)`
rust/storage/src/cell_based_row_serializer.rs	`26.66% <26.66%> (ø)`
rust/stream/src/executor/batch_query.rs	`60.86% <33.33%> (ø)`
rust/batch/src/executor/row_seq_scan.rs	`61.42% <50.00%> (ø)`
rust/storage/src/table/cell_based_table.rs	`55.59% <55.35%> (+55.59%)`	⬆️
...ust/stream/src/executor/mview/table_state_tests.rs	`96.74% <100.00%> (+<0.01%)`	⬆️
rust/stream/src/executor/mview/test_utils.rs	`100.00% <100.00%> (ø)`
rust/common/src/types/ordered_float.rs	`22.50% <0.00%> (+0.19%)`	⬆️
.../src/executor/managed_state/aggregation/extreme.rs	`90.40% <0.00%> (+0.27%)`	⬆️

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

…o wcy_cell_based_table

st1page

Rest generally LGTM, some fields in struct CellBasedTable is duplicate or unnecessary, but we can do refactor later.

st1page · 2022-03-21T06:51:08Z

rust/storage/src/table/cell_based_table.rs

+ CellBasedTableRowIter::new(self.keyspace.clone(), self.column_descs.clone(), epoch).await
+ }
+
+ pub async fn get_for_test(&self, pk: Row, column_id: i32, epoch: u64) -> Result<Option<Datum>> {


can we replaced it with get_cell? or in future PR is ok.

st1page · 2022-03-21T06:58:32Z

rust/storage/src/table/cell_based_table.rs

 keyspace: Keyspace<S>,
- pk: Vec<OrderedColumnDesc>,


we can add it back when we need it, it is for #588

sounds good

st1page · 2022-03-21T08:17:34Z

rust/storage/src/table/cell_based_table.rs

+ &mut self,
+ pk: Row,
+ cell_value: Option<Row>,
+ column_descs: Vec<ColumnDesc>,


Suggested change

column_descs: Vec<ColumnDesc>,

column_descs: &[ColumnDesc],

st1page · 2022-03-21T08:17:55Z

rust/storage/src/table/cell_based_table.rs

+ &mut self,
+ pk: Row,
+ cell_value: Option<Row>,
+ column_descs: Vec<ColumnDesc>,


Suggested change

column_descs: Vec<ColumnDesc>,

column_descs: &[ColumnDesc],

st1page · 2022-03-21T08:18:10Z

rust/storage/src/table/cell_based_table.rs

+ pub async fn batch_insert_row(
+ &mut self,
+ rows: Vec<(Row, Option<Row>)>,
+ column_descs: Vec<ColumnDesc>,


Suggested change

column_descs: Vec<ColumnDesc>,

column_descs: &[ColumnDesc],

BugenZhao

Please test the implementation well. Thanks.

rust/batch/src/executor/row_seq_scan.rs

rust/compute/tests/table_v2_materialize.rs

BugenZhao · 2022-03-21T09:47:18Z

rust/storage/src/table/cell_based_table.rs

+ let state_store_get_res = self
+ .keyspace
+ .state_store()
+ .get(&key.clone(), epoch)
+ .await
+ .unwrap();


This seems to be incorrect. The prefix of the keyspace is not prepended in this way. Have you add some tests for these functions?

Besides, what's the meaning for getting the value using pk as the key?

BugenZhao · 2022-03-21T09:49:17Z

rust/storage/src/table/cell_based_table.rs

+ let state_store_get_res = self
+ .keyspace
+ .state_store()
+ .get(&key.clone(), epoch)
+ .await
+ .unwrap();


BugenZhao · 2022-03-21T09:56:43Z

rust/storage/src/table/cell_based_table.rs

+ let column_id = column.column_id.get_id() as usize;
+ let mut cell_based_row_deserializer = CellBasedRowDeserializer::new(vec![column.clone()]);
+ let pk_and_row =
+ cell_based_row_deserializer.deserialize(&key, &state_store_get_res.unwrap())?;


Why unwrap the get result here?

BugenZhao · 2022-03-21T09:57:50Z

rust/storage/src/table/cell_based_table.rs

+ Some(pk_row) => {
+ return Ok(Some(pk_row.1.index(column_id).clone()));
+ }
+ None => Ok(None),


cell_based_row_deserializer.deserialize only returns Some when it encounter a new key. In this case, it always returns None. Please check the other usages first, for example, call deserializer.take().

BugenZhao · 2022-03-21T09:59:15Z

rust/storage/src/table/cell_based_table.rs

+ cell_based_row_deserializer.deserialize(&key, &state_store_get_res.unwrap())?;
+ match pk_and_row {
+ Some(pk_row) => {
+ return Ok(Some(pk_row.1.index(column_id).clone()));


The return value will only have one cell, given vec![column.clone()] as input. It seems incorrect to index here.

BugenZhao · 2022-03-21T09:59:35Z

rust/storage/src/table/cell_based_table.rs

+ let pk_and_row =
+ cell_based_row_deserializer.deserialize(&key, &state_store_get_res.unwrap())?;


rust/storage/src/table/cell_based_table.rs

BugenZhao · 2022-03-21T10:32:45Z

rust/storage/src/table/cell_based_table.rs

+ schema: Schema,
+
+ /// ColumnDesc contains strictly more info than `schema`.
+ column_descs: Vec<ColumnDesc>,


If we store the column_descs here, isn't passing another list of ColumnDesc for every get and insert redundant? We can simply pass some IDs there.

wcy-fdu added 3 commits March 16, 2022 21:10

draft: implement insert_row/get_row for CellBasedTable

3f24deb

draft: implement insert_row/get_row for CellBasedTable

9346414

draft: implement insert_row/get_row for CellBasedTable

2b61941

wcy-fdu marked this pull request as draft March 17, 2022 12:57

wcy-fdu changed the title ~~[WIP]feat(storage): add cell-based table~~ WIP feat(storage): add cell-based table Mar 17, 2022

wcy-fdu changed the title ~~WIP feat(storage): add cell-based table~~ [WIP] feat(storage): add cell-based table Mar 17, 2022

wcy-fdu requested review from twocode and st1page March 17, 2022 13:33

wcy-fdu added 2 commits March 18, 2022 15:23

Merge branch 'main' of github.com:singularity-data/risingwave-dev int…

dcaef23

…o wcy_cell_based_table

replace MViewTable with CellBasedTable

a26932c

wcy-fdu changed the title ~~[WIP] feat(storage): add cell-based table~~ [WIP] feat(storage): replace MViewTable with CellBasedTable and add relational interfaces Mar 19, 2022

wcy-fdu requested a review from BugenZhao March 20, 2022 14:25

wcy-fdu added 2 commits March 21, 2022 14:25

remove redundant comment

6d9126e

remove redundant comment

624e611

wcy-fdu force-pushed the wcy_cell_based_table branch from b4615aa to 624e611 Compare March 21, 2022 06:27

wcy-fdu marked this pull request as ready for review March 21, 2022 06:27

wcy-fdu added 2 commits March 21, 2022 15:30

add column_descs in relational interfaces

1af1144

merge main

7713041

st1page approved these changes Mar 21, 2022

View reviewed changes

wcy-fdu added 3 commits March 21, 2022 16:41

x

9909d90

resolve conflict

d17bccf

x

6d51b99

wcy-fdu changed the title ~~[WIP] feat(storage): replace MViewTable with CellBasedTable and add relational interfaces~~ feat(storage): replace MViewTable with CellBasedTable and add relational interfaces Mar 21, 2022

wcy-fdu merged commit dcd4b90 into main Mar 21, 2022

wcy-fdu deleted the wcy_cell_based_table branch March 21, 2022 09:20

github-actions bot added the type/feature label Mar 21, 2022

BugenZhao reviewed Mar 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): replace MViewTable with CellBasedTable and add relational interfaces #1046

feat(storage): replace MViewTable with CellBasedTable and add relational interfaces #1046

wcy-fdu commented Mar 17, 2022 •

edited

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading

st1page left a comment

st1page Mar 21, 2022

st1page Mar 21, 2022

wcy-fdu Mar 21, 2022

st1page Mar 21, 2022

st1page Mar 21, 2022

st1page Mar 21, 2022

BugenZhao left a comment

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

BugenZhao Mar 21, 2022

		let pk_and_row =
		cell_based_row_deserializer.deserialize(&key, &state_store_get_res.unwrap())?;

feat(storage): replace MViewTable with CellBasedTable and add relational interfaces #1046

feat(storage): replace MViewTable with CellBasedTable and add relational interfaces #1046

Conversation

wcy-fdu commented Mar 17, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Mar 17, 2022 • edited Loading

Codecov Report

st1page 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

BugenZhao 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

Choose a reason for hiding this comment

wcy-fdu commented Mar 17, 2022 •

edited

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading