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

[Feature][Transform][Replace]Replace support schema change #8313

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

dufeng1010
Copy link
Contributor

Purpose of this pull request

#8255

Does this PR introduce any user-facing change?

NO

@Hisoka-X Hisoka-X linked an issue Dec 17, 2024 that may be closed by this pull request
3 tasks
@Hisoka-X
Copy link
Member

Hisoka-X commented Dec 17, 2024

cc @hailin0 and @dailai

Comment on lines 44 to 47
((SeaTunnelMapTransform<SeaTunnelRow>) transformMap.get(event.tablePath().toString()))
.mapSchemaChangeEvent(event);

return event;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
((SeaTunnelMapTransform<SeaTunnelRow>) transformMap.get(event.tablePath().toString()))
.mapSchemaChangeEvent(event);
return event;
return ((SeaTunnelMapTransform<SeaTunnelRow>) transformMap.get(event.tablePath().toString()))
.mapSchemaChangeEvent(event);

Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

Please update https://github.com/apache/seatunnel/blob/dev/docs/en/concept/schema-evolution.md to let users know we can use transform with schema evolution now.

null);

@Test
public void testRepalceTransformSchangeChange() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some test case to make sure the transform fine after schema changed? For example, still can replace data as expected.

@dailai
Copy link
Contributor

dailai commented Dec 31, 2024

I think we should add some e2e case.

@Hisoka-X
Copy link
Member

I think we should add some e2e case.

Hi @dailai , I think the logic already be tested by https://github.com/apache/seatunnel/pull/8313/files#diff-835463340eff995995f88a3c8b8a25e873860a751b91b4f32322224e0346fab1R89. Should we continue add e2e test case? PS: I think we shoud move some test case from e2e to unit test. The e2e test case is too much. WDYT?

@dailai
Copy link
Contributor

dailai commented Dec 31, 2024

I think we should add some e2e case.

Hi @dailai , I think the logic already be tested by https://github.com/apache/seatunnel/pull/8313/files#diff-835463340eff995995f88a3c8b8a25e873860a751b91b4f32322224e0346fab1R89. Should we continue add e2e test case? PS: I think we shoud move some test case from e2e to unit test. The e2e test case is too much. WDYT?

UT covers logic and is not a substitute for e2e testing. The test scenario is different.

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.

[Feature][Transform][Replace] Support schema change apply
3 participants