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: minor refactor on fragment graph building #14560

Merged

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Jan 15, 2024

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

What's changed and what's your intention?

  • remove the unused field upstream_column_ids for CdcFilterNode. Because CDC source's schema is fixed.
  • add some methods to make the long CompleteStreamFragmentGraph::build_helper shorter.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch 2 times, most recently from b4d664b to d6a095f Compare January 16, 2024 14:03
@xxchan xxchan changed the base branch from main to 01-19-refactor_refactor_LogicalSource January 19, 2024 09:02
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from d6a095f to 694ebb9 Compare January 19, 2024 09:02
@xxchan xxchan force-pushed the 01-19-refactor_refactor_LogicalSource branch from de2e161 to b673a45 Compare January 19, 2024 09:23
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from 694ebb9 to de9e090 Compare January 19, 2024 09:23
@xxchan xxchan force-pushed the 01-19-refactor_refactor_LogicalSource branch from b673a45 to e5eff60 Compare January 19, 2024 14:40
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from de9e090 to 63f32da Compare January 19, 2024 14:40
@xxchan xxchan force-pushed the 01-19-refactor_refactor_LogicalSource branch from e5eff60 to 98ab255 Compare January 19, 2024 15:22
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from 63f32da to faded54 Compare January 19, 2024 15:22
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from faded54 to 4f89a84 Compare January 19, 2024 15:58
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from 4f89a84 to b440bc0 Compare January 20, 2024 05:33
@xxchan xxchan force-pushed the 01-13-refactor_minor_refactor_on_fragment_graph_building branch from b440bc0 to 8aedab6 Compare January 21, 2024 16:40
@xxchan xxchan marked this pull request as ready for review January 21, 2024 16:41
@xxchan xxchan requested review from StrikeW and BugenZhao January 21, 2024 16:41
Comment on lines 259 to 260
repeated int32 upstream_column_ids = 3;
reserved "upstream_column_ids";
reserved 3;
Copy link
Member

Choose a reason for hiding this comment

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

Since it was never used (so there was never a message persisted with #3), can we directly remove the field?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! Can you do it for me?

Comment on lines +175 to +180
self.get_table()
.unwrap()
.distribution_key
.iter()
.map(|i| *i as u32)
.collect()
Copy link
Member

Choose a reason for hiding this comment

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

We should really have a trait converting our types from and into prost types. 😄

Base automatically changed from 01-19-refactor_refactor_LogicalSource to main January 22, 2024 10:23
proto/stream_plan.proto Outdated Show resolved Hide resolved
Comment on lines +59 to 60
/// For shared CDC table on source, its `vec![]`, since the upstream source's output schema is fixed.
upstream_table_columns: HashMap<TableId, Vec<i32>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

what does the i32 mean here

@xxchan xxchan added this pull request to the merge queue Feb 1, 2024
Merged via the queue into main with commit d812316 Feb 1, 2024
28 of 30 checks passed
@xxchan xxchan deleted the 01-13-refactor_minor_refactor_on_fragment_graph_building branch February 1, 2024 05:30
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