Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(relational_state_store): define interfaces of relational layer #1900

Merged
merged 6 commits into from
Apr 20, 2022

Conversation

wcy-fdu
Copy link
Contributor

@wcy-fdu wcy-fdu commented Apr 18, 2022

What's changed and what's your intention?

As described in doc Relational State Store Layer, this PR defines IO interfaces of StateTable and MemTable.

image

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)

Comment on lines +14 to +15
#![allow(dead_code)]
#![allow(unused)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to remove these lines in the next PR. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, adding these two lines is to pass CI, and it will be removed when the interface is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should never allow unused. We can allow dead_code.

src/storage/src/table/mem_table.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #1900 (5df8e24) into main (bc61fe9) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1900      +/-   ##
==========================================
- Coverage   70.87%   70.80%   -0.07%     
==========================================
  Files         611      619       +8     
  Lines       79623    80076     +453     
==========================================
+ Hits        56431    56697     +266     
- Misses      23192    23379     +187     
Flag Coverage Δ
rust 70.80% <0.00%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
src/storage/src/table/mem_table.rs 0.00% <0.00%> (ø)
src/storage/src/table/state_table.rs 0.00% <0.00%> (ø)
src/batch/src/executor/stream_scan.rs 0.00% <0.00%> (-45.14%) ⬇️
src/common/src/array/stream_chunk_iter.rs 57.54% <0.00%> (-36.13%) ⬇️
src/common/src/test_utils/test_stream_chunk.rs 40.45% <0.00%> (-28.38%) ⬇️
src/batch/src/executor/mod.rs 62.65% <0.00%> (-21.23%) ⬇️
src/source/src/manager.rs 45.02% <0.00%> (-7.34%) ⬇️
src/meta/src/stream/graph/stream_graph.rs 61.94% <0.00%> (-4.72%) ⬇️
src/stream/src/task/mod.rs 40.69% <0.00%> (-4.18%) ⬇️
src/connector/src/utils.rs 80.28% <0.00%> (-3.55%) ⬇️
... and 120 more

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

Comment on lines 104 to 117
pub struct StateTableRowIter<S: StateStore> {
keyspace: Keyspace<S>,
/// A buffer to store prefetched kv pairs from state store
buf: Vec<(Bytes, Bytes)>,
/// The idx into `buf` for the next item
next_idx: usize,
/// A bool to indicate whether there are more data to fetch from state store
done: bool,
/// An epoch representing the read snapshot
epoch: u64,
/// Cell-based row deserializer
cell_based_row_deserializer: CellBasedRowDeserializer,
/// Statistics
_stats: Arc<StateStoreMetrics>,
Copy link
Contributor

@st1page st1page Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should re-consider the fields in StateTableRowIter. It should be a merge iterator of mem_table nad cell_based_table, so I think maybe it only contains the 2 iterators.

src/storage/src/table/mem_table.rs Outdated Show resolved Hide resolved
Self::new()
}
}
impl MemTable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A MemTable::drain(&mut self) function returns a Iterator of (pk: Row, op: RowOp) and clear the memtable.
It will be called by StateTable::commit and drain all the operators in mem_table and flush them into cell_based_table

@wcy-fdu wcy-fdu requested a review from st1page April 18, 2022 08:20
}

pub struct MemTableIter {
mem_table: MemTable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe it should be btree_map::Iter<'a, Row, RowOp>

Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally LGTM, might there will be some lifetime issue on the MemTableIter, we can resolve them while implementing them.

@wcy-fdu wcy-fdu merged commit b6dcbf9 into main Apr 20, 2022
@wcy-fdu wcy-fdu deleted the wcy_relational_layer branch April 20, 2022 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants