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

refactor(state_table): construct state_table from table_id+store instead of key_space #3593

Merged
merged 9 commits into from
Jul 4, 2022

Conversation

wcy-fdu
Copy link
Contributor

@wcy-fdu wcy-fdu commented Jul 1, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

As title, change the StateTable new() and CellBasedTable new() with table_id +store, instead of passing the whole keyspace. Top_n has three internal state, so change the frontend to assign three table_id to top_n executor.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

close #3498

@wcy-fdu wcy-fdu changed the title refactor(state_table): construct state_table from table_id+store instead of key_space[WIP] refactor(state_table): construct state_table from table_id+store instead of key_space Jul 2, 2022
@codecov
Copy link

codecov bot commented Jul 2, 2022

Codecov Report

Merging #3593 (a903286) into main (253a648) will decrease coverage by 0.00%.
The diff coverage is 84.01%.

@@            Coverage Diff             @@
##             main    #3593      +/-   ##
==========================================
- Coverage   74.30%   74.29%   -0.01%     
==========================================
  Files         773      773              
  Lines      109424   109551     +127     
==========================================
+ Hits        81304    81394      +90     
- Misses      28120    28157      +37     
Flag Coverage Δ
rust 74.29% <84.01%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/batch/src/executor/row_seq_scan.rs 13.90% <0.00%> (-0.10%) ⬇️
src/ctl/src/cmd_impl/table/scan.rs 0.00% <0.00%> (ø)
src/meta/src/stream/stream_graph.rs 77.85% <0.00%> (-1.31%) ⬇️
src/stream/src/executor/aggregation/mod.rs 76.89% <0.00%> (+1.14%) ⬆️
src/stream/src/from_proto/batch_query.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/hash_join.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/lookup.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/mview.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/top_n.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/top_n_appendonly.rs 0.00% <0.00%> (ø)
... and 27 more

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

@wcy-fdu wcy-fdu requested review from BugenZhao and st1page July 2, 2022 07:44
ctx.internal_table_id_set.insert(node.table_id_h);
}

NodeBody::AppendOnlyTopN(node) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For append_only top_n, it use two internal state table_id_l and table_id_h, I'm not sure inserting two or three table_id for append_only top_n.
cc @st1page

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 there should be two table id. but it is ok now that we waste one(not affect correctness). and later when we infer the table catalog in fe we can fix it.

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

@wcy-fdu wcy-fdu merged commit 86c2105 into main Jul 4, 2022
@wcy-fdu wcy-fdu deleted the wcy_issue_3498 branch July 4, 2022 03:39
huangjw806 pushed a commit that referenced this pull request Jul 5, 2022
…ead of key_space (#3593)

* stage 1

* refactor state table new

* refactor cell-based table new

* fix

* resolve conflict
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
…ead of key_space (risingwavelabs#3593)

* stage 1

* refactor state table new

* refactor cell-based table new

* fix

* resolve conflict
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.

refactor(relational_table): construct cellbased_table from table_id+store instead of key_space
2 participants