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] Add tidb datatype convertor #5440

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

XiaoJiang521
Copy link
Contributor

Purpose of this pull request

Check list

ic4y
ic4y previously approved these changes Sep 14, 2023
@Hisoka-X Hisoka-X changed the title [feature] add tidb datatype convertor [Feature] Add tidb datatype convertor Sep 14, 2023
@Hisoka-X
Copy link
Member

Shall we add some test case for it?

@XiaoJiang521
Copy link
Contributor Author

Shall we add some test case for it?

If need to add test cases, I can also add, but I don't know how to test this,
and only Seatunnel Web uses this datatypeconvorter,

@Hisoka-X
Copy link
Member

Shall we add some test case for it?

If need to add test cases, I can also add, but I don't know how to test this, and only Seatunnel Web uses this datatypeconvorter,

Got it.

return toSeaTunnelType(mysqlType, dataTypeProperties);
}

// todo: It's better to wrapper MysqlType to a pojo in ST, since MysqlType doesn't contains
Copy link
Member

Choose a reason for hiding this comment

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

contains what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contains properties. This place uses MysqlDataTypeConvertor, so take the annotations too QAQ

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

+1

@ruanwenjun ruanwenjun merged commit 61391bd into apache:dev Sep 15, 2023
47 checks passed
gnehil pushed a commit to gnehil/seatunnel that referenced this pull request Oct 12, 2023
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.

4 participants