-
Notifications
You must be signed in to change notification settings - Fork 13
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 support #285
base: main
Are you sure you want to change the base?
add Mysql support #285
Conversation
@matthewmturner, what do you think about this feature? I will resolve conflict tomorrow. |
@@ -16,6 +16,7 @@ datafusion-functions-parquet = { version = "0.1.0", path = "..//datafusion-funct | |||
datafusion-udfs-wasm = { version = "0.1.0", path = "../datafusion-udfs-wasm", features = [ | |||
"serde", | |||
], optional = true } | |||
datafusion-table-providers = { git = "https://github.com/jychen7/datafusion-table-providers", branch = "datafusion-45", features = ["mysql"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like datafusion-table-providers
needs to update to DF v45. Would you be able to make that change first and see when they plan on doing release? I am trying to align all my DF related dependencies on v45 for the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this PR is using a temp branch jychen7/datafusion-table-providers#1, combining datafusion-contrib/datafusion-table-providers#249 and datafusion-contrib/datafusion-table-providers#247.
I agree we should wait those two PRs merged, that's why this is a draft. I will post in Slack or Discord and ask if maintainer have time to help review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn default_mysql_database() -> String { | ||
"".to_string() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that familiar with MySql but im surprised the default database is an empty string - is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, we should remove default. MySQL does not have default database for applications and when using mysql
client, we need to say use {database};
I love this! I left a couple comments. One thing in particular that is missing is a test for the extension. Would you be able to add a test by following the same pattern that we already have setup for testing extensions? |
sounds good |
verify locally with
cargo run --features=mysql
example config
please refer to https://github.com/datafusion-contrib/datafusion-table-providers?tab=readme-ov-file#mysql for local mysql setup