[FLINK-38738] Don't overwrite transformation names by advanced ScanProviders #27278
Conversation
…oviders Advanced ScanProviders may return multiple transformations and hence overwriting just the last name is meaningless and potentially dangerous. It is fair to assume that the users of the advanced ScanProviders set names and uids on their own.
| ((DataStreamScanProvider) provider) | ||
| .produceDataStream(createProviderContext(config), env) | ||
| .getTransformation(); | ||
| meta.fill(sourceTransform); |
There was a problem hiding this comment.
I am curious - could we add a test that has the advanced ScanProviders "setting names and uids on their own". I think it would be useful, if this is expected practise.
There was a problem hiding this comment.
I agree with David here. This PR needs proper testing in org.apache.flink.table.planner.plan.nodes.exec.TransformationsTest
github-actions
bot
added
the
community-reviewed
There was a problem hiding this comment.
Thank you for the PR @AHeise. In general, I agree with the changes. But we need more tests. This is a serious bug that will need good release notes. Maybe we should also offer table option to restore the old behavior until sources are updated.
| ((DataStreamScanProvider) provider) | ||
| .produceDataStream(createProviderContext(config), env) | ||
| .getTransformation(); | ||
| meta.fill(sourceTransform); |
There was a problem hiding this comment.
I agree with David here. This PR needs proper testing in org.apache.flink.table.planner.plan.nodes.exec.TransformationsTest
|
This issue is larger than originally thought. I assigned this ticket to me and will close this PR. |
4 participants
What is the purpose of the change
Advanced ScanProviders may return multiple transformations and hence overwriting just the last name is meaningless and potentially dangerous. It is fair to assume that the users of the advanced ScanProviders set names and uids on their own.
Brief change log
Verifying this change
Trivial fix (deletion of 2 lines).
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation