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: remove keyspace #6406

Merged
merged 10 commits into from
Nov 17, 2022
Merged

refactor: remove keyspace #6406

merged 10 commits into from
Nov 17, 2022

Conversation

Gun9niR
Copy link
Contributor

@Gun9niR Gun9niR commented Nov 16, 2022

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

What's changed and what's your intention?

After #6130, keys are no longer "globally unique" - keys of different operators are distinguished by different table_ids in the read/write option when calling state store interface. The table_id prefix is no longer required, making KeySpace a redundant layer. So this PR removes it.

  • Remove keyspace in code, comments, and docs.
  • ExtractTableKeyIter is moved to table/mod.rs, and is constructed in StorageIterInner

Checklist

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

Documentation

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

  • Installation and deployment
  • Connector (sources & sinks)
  • SQL commands, functions, and operators
  • RisingWave cluster configuration changes
  • Other (please specify in the release note below)

Release note

Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.

Refer to a related PR or issue link (optional)

@Gun9niR Gun9niR marked this pull request as draft November 16, 2022 14:06
@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #6406 (05fbe6c) into main (ddae931) will decrease coverage by 0.16%.
The diff coverage is 54.96%.

@@            Coverage Diff             @@
##             main    #6406      +/-   ##
==========================================
- Coverage   74.28%   74.12%   -0.17%     
==========================================
  Files         960      966       +6     
  Lines      156525   158029    +1504     
==========================================
+ Hits       116277   117141     +864     
- Misses      40248    40888     +640     
Flag Coverage Δ
rust 74.12% <54.96%> (-0.17%) ⬇️

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 20.20% <ø> (-34.05%) ⬇️
src/batch/src/task/task_execution.rs 50.29% <0.00%> (ø)
src/common/src/types/chrono_wrapper.rs 46.66% <0.00%> (-15.91%) ⬇️
src/common/src/types/interval.rs 72.10% <0.00%> (-7.47%) ⬇️
src/common/src/types/mod.rs 72.40% <0.00%> (ø)
src/ctl/src/cmd_impl/table/scan.rs 0.00% <0.00%> (ø)
src/expr/src/expr/build_expr_from_prost.rs 53.76% <0.00%> (-1.91%) ⬇️
src/expr/src/expr/expr_binary_nonnull.rs 62.80% <0.00%> (-8.89%) ⬇️
src/expr/src/expr/mod.rs 47.91% <0.00%> (-1.02%) ⬇️
src/expr/src/vector_op/date_trunc.rs 0.00% <0.00%> (ø)
... and 158 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@wcy-fdu wcy-fdu 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! Thanks for the PR.

Copy link
Contributor

@wenym1 wenym1 left a comment

Choose a reason for hiding this comment

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

Rest LGTM

iter: I,
}

impl<I: StateStoreIter<Item = (FullKey<Vec<u8>>, Bytes)>> StateStoreIter
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try implementing a map method for StateStoreIter by adding a trait named like StateStoreIterMapExt?

@Gun9niR Gun9niR marked this pull request as ready for review November 17, 2022 09:51
@mergify mergify bot merged commit fdf61cf into main Nov 17, 2022
@mergify mergify bot deleted the zhidong/remove-keyspace branch November 17, 2022 10:28
xxchan pushed a commit that referenced this pull request Nov 19, 2022
* remove all functions in keyspace

* remove keyspace

* misc

* revert version.rs

* rename store to local_store in streaming table

* replace ExtractTableKeyIterator with a more general interface

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

4 participants