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: support altering the target table’s columns of the sink. #17203

Merged
merged 6 commits into from
Jul 31, 2024

Conversation

shanicky
Copy link
Contributor

@shanicky shanicky commented Jun 11, 2024

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

What's changed and what's your intention?

Title: Enhance Sink Behavior to Handle Table Alterations and Add Associated Tests

Summary:

This pull request implements enhancements to the system's behavior concerning sinks when table alterations occur. The changes span across several areas of the codebase, including test scripts, protobuf definitions, and Rust source code. Here's a breakdown of the key additions and modifications:

  • A new end-to-end test script alter_column.slt has been added under e2e_test/sink/sink_into_table. This script validates the system's ability to manage sinks during various table alteration operations, including creating tables and sinks, inserting values, and adding or attempting to drop columns. It further checks if the sink tables reflect the correct state after these operations.

  • Removal of a specific test case from basic.slt indicates an important change in the behavior of the system. The removed test case previously checked for an error when altering a table by adding a column with active incoming sinks. This change implies that the system may now support adding columns to a populated sink table.

  • The protobuf definition for a Sink within catalog.proto has been updated to incorporate a repeated field original_target_columns. This new field stores the initial structure of a sink's target table, facilitating the management of table alterations in relation to sinks.

  • The main body of Rust source code changes revolves around handling table alterations when sinks are involved:

    • The modules sink/catalog/desc.rs and sink/catalog/mod.rs have been updated to manage the new original_target_columns data.
    • A new function called default_column_exprs has been added to frontend/src/catalog/table_catalog.rs. This function generates default expressions for columns, which is essential for populating default values in rows added to altered tables.
    • The code within frontend/src/handler/alter_table_column.rs has been augmented to enable table alterations in the presence of sinks, highlighting improved support for adding columns to tables with active sinks.
  • The pull request also notes the use of Rust HashMap and Vec collections to maintain the newly introduced data points, signaling improvements in data handling, particularly for append-only tables engaged in sink operations.

This pull request presents a comprehensive enhancement to the system, focusing on improved sink interaction with table alterations. Reviewers can delve into the individual elements of this update for a more detailed understanding, and further discussions are encouraged to fine-tune these changes.

Please review these changes and provide feedback on any aspects that may benefit from further clarification

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)

@shanicky shanicky force-pushed the peng/alter-sink-target-table branch 2 times, most recently from fd985ab to 4679d4d Compare June 12, 2024 15:50
@shanicky shanicky marked this pull request as ready for review June 13, 2024 16:53
@shanicky shanicky changed the title [wip] feat: support altering the target table’s columns of the sink. feat: support altering the target table’s columns of the sink. Jun 18, 2024
@risingwavelabs risingwavelabs deleted a comment from gitguardian bot Jun 18, 2024
Copy link

gitguardian bot commented Jun 20, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

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.

original_target_columns compatibility code is scattered across the meta and frontend, and only performs compatibility after changing the schema of the table. Seems a bit easy to get wrong. Can we execute the compatibility code on meta startup to fill all sink values?

proto/catalog.proto Outdated Show resolved Hide resolved
src/connector/src/sink/catalog/mod.rs Show resolved Hide resolved
src/frontend/src/handler/alter_table_column.rs Outdated Show resolved Hide resolved
src/frontend/src/handler/alter_table_column.rs Outdated Show resolved Hide resolved
src/meta/src/controller/streaming_job.rs Show resolved Hide resolved
@shanicky shanicky force-pushed the peng/alter-sink-target-table branch from 872fe27 to ecabd06 Compare July 2, 2024 09:41
@shanicky shanicky requested a review from yezizp2012 July 3, 2024 09:22
@shanicky shanicky force-pushed the peng/alter-sink-target-table branch from 926593b to cdc7696 Compare July 3, 2024 09:24
@shanicky shanicky requested a review from st1page July 3, 2024 09:24
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.

LGTM

src/meta/src/manager/catalog/mod.rs Show resolved Hide resolved
input: vec![StreamNode {
node_body: Some(NodeBody::Merge(MergeNode {
..Default::default()
})),
..Default::default()
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be clear to explicitly populate these defaults value and add an todo comment here? #17658

@shanicky shanicky force-pushed the peng/alter-sink-target-table branch 6 times, most recently from 47a6563 to e8178e4 Compare July 30, 2024 09:02
@shanicky
Copy link
Contributor Author

It seems like there's no way to resolve the single node test stackoverflow issue. 🥵

@st1page
Copy link
Contributor

st1page commented Jul 30, 2024

It seems like there's no way to resolve the single node test stackoverflow issue. 🥵

:lark_cry

@fuyufjh
Copy link
Member

fuyufjh commented Jul 31, 2024

Shall we try rust_min_stack

@shanicky shanicky force-pushed the peng/alter-sink-target-table branch from 74d0c29 to 66b79d7 Compare July 31, 2024 07:07
@shanicky shanicky added this pull request to the merge queue Jul 31, 2024
Merged via the queue into main with commit ce397f9 Jul 31, 2024
33 checks passed
@shanicky shanicky deleted the peng/alter-sink-target-table branch July 31, 2024 09:29
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