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

Add MySQL write support #134

Merged
merged 9 commits into from
Nov 25, 2024
Merged

Conversation

gengteng
Copy link
Contributor

Hey @phillipleblanc ,

I’ve got an initial version of the MySQL write support ready. No tests or manual validation yet, but I wanted to get this PR up and continue working on those next.

Cheers!

@gengteng
Copy link
Contributor Author

Summary of Changes

  1. MySQL Write Support:

    • Implemented MySQLTableWriter and MySQLDataSink to enable inserting data into MySQL.
  2. DataType Handling:

    • For DataType::Struct, similar to SQLite, the data is stored in JSON format.
    • Mapped DataType::Binary and DataType::LargeBinary to ColumnType::Blob.
  3. Refactor:

    • Moved the to_datafusion_error function to the util module for better organization.

Dependency

@gengteng gengteng changed the title Add initial MySQL write support (WIP) Add initial MySQL write support Oct 15, 2024
@gengteng gengteng changed the title Add initial MySQL write support Add MySQL write support Oct 15, 2024
@phillipleblanc
Copy link
Collaborator

@gengteng I'll look at reviewing this PR over the next week. Do you know why the integration test is failing?

@gengteng
Copy link
Contributor Author

Hi @phillipleblanc ,

The integration test is failing because it depends on another PR, feat: add support for converting LargeUtf8/LargeString in try_cast_to (datafusion-federation#71). The test code in my PR uses changes introduced in that PR. Once it’s merged and released, the test should pass.

If you could help push for that PR to be merged, it would be greatly appreciated. Alternatively, I could extract the relevant logic into this PR, but that would result in code duplication.

Or do you have any better suggestions?

Thank you for reviewing!

@phillipleblanc
Copy link
Collaborator

I've reviewed the PR in datafusion-federation

Copy link
Collaborator

@phillipleblanc phillipleblanc left a comment

Choose a reason for hiding this comment

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

Nice PR! I have a couple of comments, but this is looking good.

src/mysql.rs Show resolved Hide resolved
@gengteng
Copy link
Contributor Author

resolve #122

@holicc
Copy link
Contributor

holicc commented Nov 21, 2024

@gengteng Is this PR still in progress? I recently needed this feature, and if there's anything I can do to help, I'd be happy to assist. :)

@gengteng
Copy link
Contributor Author

@gengteng Is this PR still in progress? I recently needed this feature, and if there's anything I can do to help, I'd be happy to assist. :)

Hi @holicc,

Thank you so much for offering to help—I really appreciate it! 😊

I’ve resolved all issues locally and merged the latest version. All tests pass successfully on my local machine. However, the tests are still failing on GitHub Actions, possibly due to memory or disk space limitations (exit code 143).

Could you please help check if there are resource constraints or other underlying issues? Thanks again for your assistance!

@holicc
Copy link
Contributor

holicc commented Nov 25, 2024

@gengteng Is this PR still in progress? I recently needed this feature, and if there's anything I can do to help, I'd be happy to assist. :)

Hi @holicc,

Thank you so much for offering to help—I really appreciate it! 😊

I’ve resolved all issues locally and merged the latest version. All tests pass successfully on my local machine. However, the tests are still failing on GitHub Actions, possibly due to memory or disk space limitations (exit code 143).

Could you please help check if there are resource constraints or other underlying issues? Thanks again for your assistance!

Yes, all tests have passed on my laptop too. It seems the problem is not with the code. @phillipleblanc, can you retry the actions?

@phillipleblanc
Copy link
Collaborator

Thanks @gengteng!

@phillipleblanc phillipleblanc merged commit 6e42290 into datafusion-contrib:main Nov 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants