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.

Sign up for GitHub

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

Jump to bottom

[FLINK-36093][transform] fix preTransformoperator wrongly filters columns when multiple transform #3572

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[FLINK-36093][transform] fix preTransformoperator wrongly filters columns when multiple transform #3572

wants to merge 7 commits into from
+387 −114

Conversation

MOBIN-F
Copy link
Contributor

@MOBIN-F MOBIN-F commented Aug 24, 2024

Currently, such transform rule could not work:

transform:
projection: 'A' as result
filter: tag >= 0
projection: score as result
filter: tag < 0

Here, score column will be filtered out in PreTransform stage, since it wasn't referenced in the first transform rule. As a result, the following transform rule will fail since score does not exist in PostTransform operator.

@MOBIN-F MOBIN-F force-pushed the fix_PreTransform_operator_wrongly_filters_out_columns_when_multiple_transform_rules branch from e2edbe6 to aed5494 Compare August 25, 2024 16:41
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity for 60 days. It will be closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Oct 25, 2024
yuxiqian
yuxiqian reviewed Oct 25, 2024
View reviewed changes
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.

Thanks for @MOBIN-F's contribution! Apart from great unit tests of transform operators, some integrated / E2e tests will be nice in FlinkPipelineTransformITCase and TransformE2eITCase.

leonardBang
leonardBang approved these changes Nov 7, 2024
View reviewed changes
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 @MOBIN-F for the contribution, I think current unit test is okay for me. Sure that future e2e test is welcome

@leonardBang
Copy link
Contributor

@MOBIN-F Could you help rebase this PR?

@MOBIN-F
Merge remote-tracking branch 'origin/master' into fix_PreTransform_op…
1ac2992 
…erator_wrongly_filters_out_columns_when_multiple_transform_rules
@MOBIN-F
Copy link
Contributor Author

MOBIN-F commented Nov 7, 2024

@MOBIN-F Could you help rebase this PR?
@leonardBang done~

@MOBIN-F
Copy link
Contributor Author

MOBIN-F commented Nov 8, 2024

CI failure,It seems that this PR(#3696) should be merged first

@leonardBang
Copy link
Contributor

@MOBIN-F Could you kindly rebase again?

@MOBIN-F
Merge remote-tracking branch 'origin/master' into fix_PreTransform_op…
b727b6b 
…erator_wrongly_filters_out_columns_when_multiple_transform_rules
@MOBIN-F
Copy link
Contributor Author

MOBIN-F commented Nov 8, 2024

@MOBIN-F Could you kindly rebase again?

done~

@MOBIN-F
Copy link
Contributor Author

MOBIN-F commented Nov 8, 2024

CI failure,I will recheck the code

@MOBIN-F
Copy link
Contributor Author

MOBIN-F commented Nov 11, 2024

I previously ignored the scenario where projection was missing. I fixed it and added integrated / E2e tests , Can you review it again? @yuxiqian
In my CI Aaction,the tests passed
image

yuxiqian
yuxiqian reviewed Nov 11, 2024
View reviewed changes
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.

Thanks for cleaning up PreTransformOperator! Looks much neater now, just left some trivial comments.

Also +@aiwenmo who is familiar with transform modules.

...ime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/PreTransformOperator.java Outdated Show resolved Hide resolved
yuxiqian
yuxiqian reviewed Nov 11, 2024
View reviewed changes
...src/test/java/org/apache/flink/cdc/runtime/operators/transform/PreTransformOperatorTest.java
@Test
void testMultiTransform() throws Exception {
PreTransformOperator transform =
PreTransformOperator.newBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can also cover cases when projection is * or omitted here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~, also above
added some tests

@aiwenmo
Copy link
Contributor

aiwenmo commented Nov 12, 2024

Thanks for cleaning up PreTransformOperator! Looks much neater now, just left some trivial comments.

Also +@aiwenmo who is familiar with transform modules.

Hi. @MOBIN-F. Thanks for the contribution. And I will assist in reviewing the code tonight~

aiwenmo
aiwenmo reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@aiwenmo aiwenmo left a comment

Choose a reason for hiding this comment

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

LGTM.

@MOBIN-F
Copy link
Contributor Author

MOBIN-F commented Nov 13, 2024

oceanbase tests failure,but it seems to have nothing to do with this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuxiqian yuxiqian yuxiqian left review comments

@aiwenmo aiwenmo aiwenmo left review comments

@leonardBang leonardBang leonardBang approved these changes

Assignees
No one assigned
Labels
approved composer e2e-tests reviewed runtime Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MOBIN-F @leonardBang @aiwenmo @yuxiqian