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

[FLINK-36981][cdc] Fix the bug of transform merging with route in YAML task #3826

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

aiwenmo
Copy link
Contributor

@aiwenmo aiwenmo commented Dec 30, 2024

Situation

In FlinkPipelineComposerITCase.testTransformMergingWithRoute:

// Create test dataset:
// Create table 1 [id, name, age]
// Table 1: +I[1, Alice, 18]
// Table 1: +I[2, Bob, 20]
// Table 1: -U[2, Bob, 20] +U[2, Bob, 30]
// Create table 2 [id, name, age, description]
// Table 2: +I[3, Charlie, 15, student]
// Table 2: +I[4, Donald, 25, student]
// Table 2: -D[4, Donald, 25, student]
// Rename column for table 1: name -> last_name
// Add column for table 2: gender
// Table 1: +I[5, Eliza, 24]
// Table 2: +I[6, Frank, 30, student, male] 

This is executed correctly.

// Create test dataset:
// Create table 1 [id, name, age]
// Create table 2 [id, name, age, description]
// Table 1: +I[1, Alice, 18]
// Table 1: +I[2, Bob, 20]
// Table 1: -U[2, Bob, 20] +U[2, Bob, 30]
// Table 2: +I[3, Charlie, 15, student]
// Table 2: +I[4, Donald, 25, student]
// Table 2: -D[4, Donald, 25, student]
// Rename column for table 1: name -> last_name
// Add column for table 2: gender
// Table 1: +I[5, Eliza, 24]
// Table 2: +I[6, Frank, 30, student, male] 

This will cause an exception.

Caused by: org.codehaus.commons.compiler.CompileException: Line 1, Column 76: Assignment conversion not possible from type "java.lang.Integer" to type "java.lang.Long" 

Cause

When a conversion rule of transform matches multiple tables, only tables with the same structure are supported. However, in the scenario of sharding databases and tables, it is possible to match multiple tables with different structures.
In the Map<Tuple2<TableId, TransformProjection>, TransformProjectionProcessor> transformProjectionProcessorMap ,TransformProjection is wrongly shared, resulting in Schema always having the schema of the latest matched table.

Fix

1.Change Map<Tuple2<TableId, TransformProjection>, TransformProjectionProcessor> transformProjectionProcessorMap into Map<Tuple2<TableId, String>, TransformProjectionProcessor> transformProjectionProcessorMap.
2.When transformProjectionProcessor has none TableInfo, set TransformProjection into new transformProjectionProcessor from existed transformProjectionProcessor.

@aiwenmo
Copy link
Contributor Author

aiwenmo commented Dec 31, 2024

PTAL @yuxiqian @lvyanquan

@yuxiqian
Copy link
Contributor

yuxiqian commented Jan 2, 2025

1.Change Map<Tuple2<TableId, TransformProjection>, TransformProjectionProcessor> transformProjectionProcessorMap into Map<Tuple2<TableId, String>, TransformProjectionProcessor> transformProjectionProcessorMap.

Seems you're changing the key of this map from [TableId, TransformProjection] to [Transform, TransformProjection#projection]. However the problem is Projection string is a field of TransformProjection. If TransformProjection might collide among different rules, how can the ProjectionString be unique?


Off topic, but I doubt if transform module has to be that complicated.

Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

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

As discussed with @aiwenmo, we'll keep this PR minimal, focusing on bug-fix and leave transform module cleaning-up work later.

Could @leonardBang please trigger the CI?

Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

Thanks @aiwenmo for the contribution and @yuxiqian for the review, LGTM

@leonardBang leonardBang merged commit 49dc957 into apache:master Jan 9, 2025
30 checks passed
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