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

feat(sink): infer internal table for kv log store in sink #10065

Merged
merged 12 commits into from
Jul 14, 2023

Conversation

xx01cyx
Copy link
Contributor

@xx01cyx xx01cyx commented May 30, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Infer the internal table for kv log store in sink. The sink executor can construct a kv log store based on the internal table.

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
  • All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

  • I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

  • My PR DOES NOT contain user-facing changes.
Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

  • 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

@xx01cyx xx01cyx requested review from wenym1 and st1page May 30, 2023 07:56
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #10065 (a195a48) into main (33071ff) will decrease coverage by 0.01%.
The diff coverage is 57.40%.

@@            Coverage Diff             @@
##             main   #10065      +/-   ##
==========================================
- Coverage   69.95%   69.94%   -0.01%     
==========================================
  Files        1307     1307              
  Lines      223475   223612     +137     
==========================================
+ Hits       156327   156410      +83     
- Misses      67148    67202      +54     
Flag Coverage Δ
rust 69.94% <57.40%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/constants.rs 25.00% <ø> (ø)
src/common/src/util/stream_graph_visitor.rs 50.60% <0.00%> (-0.94%) ⬇️
src/frontend/src/optimizer/plan_node/stream.rs 13.75% <0.00%> (-0.04%) ⬇️
src/meta/src/hummock/metrics_utils.rs 70.61% <0.00%> (-3.33%) ⬇️
src/meta/src/manager/catalog/fragment.rs 27.85% <0.00%> (-0.35%) ⬇️
src/meta/src/rpc/server.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/sink.rs 0.00% <0.00%> (ø)
src/meta/src/hummock/manager/mod.rs 68.16% <29.62%> (-0.27%) ⬇️
src/meta/src/rpc/metrics.rs 78.54% <63.33%> (-0.71%) ⬇️
...rc/frontend/src/optimizer/plan_node/stream_sink.rs 75.10% <100.00%> (+4.85%) ⬆️
... and 4 more

... and 10 files with indirect coverage changes

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

@xx01cyx xx01cyx requested a review from st1page July 5, 2023 08:45
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.

IIRC during our discussion, we need a vnode column? @wenym1

@xx01cyx xx01cyx added this pull request to the merge queue Jul 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 12, 2023
.add_order_column(SEQ_ID_COLUMN_INDEX, OrderType::ascending_nulls_last());
let read_prefix_len_hint = table_catalog_builder.get_current_pk_len();

let value_indices = table_catalog_builder.extend_columns(&self.sink_desc().columns);
Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried running with kv log store. It fails because the value_indices of predefined columns are not included here.

We should fix it before merging into main.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

@wenym1 wenym1 added this pull request to the merge queue Jul 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 13, 2023
@wenym1 wenym1 added this pull request to the merge queue Jul 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 13, 2023
@wenym1 wenym1 added this pull request to the merge queue Jul 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 13, 2023
@wenym1 wenym1 enabled auto-merge July 13, 2023 11:18
@wenym1 wenym1 added this pull request to the merge queue Jul 14, 2023
Merged via the queue into main with commit a541ec6 Jul 14, 2023
@wenym1 wenym1 deleted the cyx/kv-log-store-table branch July 14, 2023 07:17
kwannoel pushed a commit that referenced this pull request Jul 14, 2023
Co-authored-by: William Wen <william123.wen@gmail.com>
Co-authored-by: William Wen <44139337+wenym1@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.

3 participants