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.

Sign up for GitHub

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

Jump to bottom

refactor(lookup): use state table for lookup #3570

Merged
merged 3 commits into from
Jul 2, 2022
Merged

refactor(lookup): use state table for lookup #3570

merged 3 commits into from
Jul 2, 2022

Conversation

BowenXiao1999
Copy link
Contributor

@BowenXiao1999 BowenXiao1999 commented Jun 30, 2022 •
edited

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

What's changed and what's your intention?

As title.

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)

@BowenXiao1999 BowenXiao1999 marked this pull request as draft June 30, 2022 06:36
BowenXiao1999
BowenXiao1999 commented Jun 30, 2022
View reviewed changes
src/stream/src/executor/lookup/impl_.rs
Comment on lines -169 to -172
let arrangement_order_types = arrangement_order_rules
.iter()
.map(|x| x.order_type)
.collect();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This arrangement_order_types do not includes pk but only sort key. This semantics is different from Materialize executor. I investigate and found that it's because we use arrange_key here:

https://github.com/singularity-data/risingwave/blob/e26eb0c0218f499a25c7c35e4ee0315c24eca2f1/src/stream/src/from_proto/lookup.rs#L35-L40

Materialize use directly arrangement order types in tableinfo.

https://github.com/singularity-data/risingwave/blob/e26eb0c0218f499a25c7c35e4ee0315c24eca2f1/src/stream/src/from_proto/mview.rs#L78-L83

Because future state table catalog may comes from frontend, I keep it unchanged in this PR.

@BugenZhao BugenZhao requested a review from skyzh June 30, 2022 07:49
@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_state_table_for_lookup branch 4 times, most recently from aea0038 to cdf3127 Compare June 30, 2022 08:06
BowenXiao1999
BowenXiao1999 commented Jun 30, 2022
View reviewed changes
src/stream/src/executor/lookup/impl_.rs
// The key is truncated in `arrange_keyspace` so there's no vnode to decode
// TODO: refactor lookup with cell-based table and remove `deserialize_with_vnode`
for (pk_with_cell_id, cell) in all_cells {
tracing::trace!(target: "events::stream::lookup::scan", "{:?} => {:?}", pk_with_cell_id, cell);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the tracing cuz I think it will include too much noise?

@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_state_table_for_lookup branch 2 times, most recently from ffaa3d6 to 8df50b0 Compare June 30, 2022 08:46
@BowenXiao1999 BowenXiao1999 marked this pull request as ready for review June 30, 2022 08:47
@BowenXiao1999 BowenXiao1999 changed the title refactor: use state table for lookup refactor(lookup): use state table for lookup Jun 30, 2022
@codecov
Copy link

codecov bot commented Jun 30, 2022 •
edited

Codecov Report

Merging #3570 (f3abac1) into main (d187c37) will decrease coverage by 0.00%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #3570      +/-   ##
==========================================
- Coverage   74.30%   74.29%   -0.01%     
==========================================
  Files         773      773              
  Lines      109438   109424      -14     
==========================================
- Hits        81321    81300      -21     
- Misses      28117    28124       +7
Flag Coverage Δ
rust 74.29% <91.66%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/stream/src/executor/lookup/sides.rs 29.16% <ø> (ø)
src/stream/src/executor/lookup/impl_.rs 95.80% <91.66%> (-0.33%) ⬇️
src/storage/src/cell_based_row_deserializer.rs 88.00% <0.00%> (-4.58%) ⬇️
src/meta/src/manager/id.rs 96.06% <0.00%> (+0.56%) ⬆️

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

@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_state_table_for_lookup branch from 8df50b0 to 8e48509 Compare July 1, 2022 03:13
@BugenZhao BugenZhao self-requested a review July 1, 2022 11:38
@skyzh
Copy link
Contributor

skyzh commented Jul 2, 2022

Surprised to see so little changes are okay to get this migration done 🤣

@skyzh
Copy link
Contributor

skyzh commented Jul 2, 2022

... except that some wrong information in my code docs might have slowed down the process 😇

@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_state_table_for_lookup branch from 8e48509 to f6101c7 Compare July 2, 2022 06:39
@BowenXiao1999
Copy link
Contributor Author

... except that some wrong information in my code docs might have slowed down the process 😇

I may open some refactor to make some semantics more clear, e.g. #3570 (comment).

@BowenXiao1999 BowenXiao1999 enabled auto-merge (squash) July 2, 2022 06:42
@skyzh
Copy link
Contributor

skyzh commented Jul 2, 2022

Please use merge queue to auto-merge. Add can-merge tag.

@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_state_table_for_lookup branch from f6101c7 to f3abac1 Compare July 2, 2022 15:21
@BowenXiao1999 BowenXiao1999 added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jul 2, 2022
@mergify mergify bot merged commit 253a648 into main Jul 2, 2022
@mergify mergify bot deleted the bw/use_state_table_for_lookup branch July 2, 2022 15:33
huangjw806 pushed a commit that referenced this pull request Jul 5, 2022
@BowenXiao1999 @huangjw806
refactor(lookup): use state table for lookup (#3570)
99e0ef5 
* refactor: use state table for lookup

* clean

* fix clone
@BowenXiao1999 BowenXiao1999 mentioned this pull request Jul 5, 2022
Closed
@BowenXiao1999 BowenXiao1999 mentioned this pull request Jul 25, 2022
Closed
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
@BowenXiao1999 @nasnoisaac
refactor(lookup): use state table for lookup (risingwavelabs#3570)
7efe444 
* refactor: use state table for lookup

* clean

* fix clone
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwannoel kwannoel kwannoel left review comments

@skyzh skyzh skyzh approved these changes

@BugenZhao BugenZhao Awaiting requested review from BugenZhao

Assignees
No one assigned
Labels
mergify/can-merge Indicates that the PR can be added to the merge queue type/refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

streaming: refactor look-up of delta join with cell-based table
3 participants
@BowenXiao1999 @skyzh @kwannoel