Skip to content

refactor: remove col_index_mapping when replacing table #21685

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

Open
wants to merge 1 commit into
base: bz/dispatcher-output-mapping-rewrite-index
Choose a base branch
from

Conversation

BugenZhao
Copy link
Member

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

What's changed and what's your intention?

#21499 eliminated the need for using ColIndexMapping for deriving the dispatcher after replacing a table. In last PR (#21681), we further eliminated the need for using it to rewrite the indexes. As a result, we can remove this field from the request of ReplaceTable now.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from 19486df to e151155 Compare May 2, 2025 05:55
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch 2 times, most recently from 5a1b638 to ff20219 Compare May 2, 2025 06:44
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from e151155 to 5277b6b Compare May 2, 2025 06:44
@BugenZhao BugenZhao marked this pull request as ready for review May 2, 2025 07:10
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch from ff20219 to 9c4fd0f Compare May 2, 2025 07:10
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from 5277b6b to 892cb56 Compare May 2, 2025 07:10
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch from 9c4fd0f to e7b889e Compare May 2, 2025 08:15
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from 892cb56 to a178ce1 Compare May 2, 2025 08:15
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the now-unnecessary col_index_mapping parameter and related logic from table replacement handling. Key changes include removing the use of ColIndexMapping in multiple modules (RPC client, meta, and frontend) as well as updating proto definitions accordingly.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/rpc_client/src/meta_client.rs Removed ColIndexMapping usage from replace_job and related calls
src/meta/src/rpc/ddl_controller.rs Eliminated col_index_mapping parameter from job replacement API
src/meta/src/controller/streaming_job.rs Removed col_index_mapping parameters in job finishing methods
src/meta/service/src/ddl_service.rs Dropped col_index_mapping processing in extracting replace table info
src/frontend (various files) Updated function signatures and calls to omit col_index_mapping
proto/ddl_service.proto Removed table_col_index_mapping field from proto request messages
Comments suppressed due to low confidence (2)

src/rpc_client/src/meta_client.rs:642

  • Since ColIndexMapping is no longer needed in the ReplaceJobPlanRequest, ensure that all internal and external documentation is updated to reflect that this parameter is removed.
table_col_index_mapping: ColIndexMapping,

proto/ddl_service.proto:372

  • Remove or update comments referencing table_col_index_mapping so that the proto definition clearly reflects the new API without this field.
catalog.ColIndexMapping table_col_index_mapping = 2;

@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch from e7b889e to f7de5e4 Compare May 6, 2025 06:44
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from a178ce1 to 2ed7553 Compare May 6, 2025 06:44
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch from f7de5e4 to 86eb5d7 Compare May 13, 2025 07:13
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from 2ed7553 to 0a3cc8a Compare May 13, 2025 07:13
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch from 86eb5d7 to 4696b1c Compare May 14, 2025 06:04
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from 0a3cc8a to 1534733 Compare May 14, 2025 06:04
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-remove-col-index-mapping branch from 1534733 to 9a9f63c Compare May 14, 2025 06:11
@BugenZhao BugenZhao force-pushed the bz/dispatcher-output-mapping-rewrite-index branch from 4696b1c to a193a49 Compare May 14, 2025 06:11
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