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 copy_to2 #72

Merged
merged 6 commits into from
Oct 13, 2024
Merged

add copy_to2 #72

merged 6 commits into from
Oct 13, 2024

Conversation

vituri
Copy link
Contributor

@vituri vituri commented Oct 10, 2024

I messed a bit with the project.toml to add CSV and MySQL; I also created the file copy_to.jl where I wrote a draft of how to make inserts in MariaDB from julia dataframes. I called my new function copy_to2 to avoid clashes with the other one.

Its arguments are

function copy_to2(df::DataFrames.AbstractDataFrame, table_name, con::MySQL.Connection; replace = false)

where we need a dataframe, a table_name (in the database), a connection (for now, just the MySQL/MariaDB), and the replace argument (which is false by default).

@drizk1
Copy link
Member

drizk1 commented Oct 10, 2024

Thank you so much, this looks great. Idk why the documenter is failing, this is something with a recent julia or documenter update because I am newly having this issue as well.

As for the PR, a few small changes if you dont mind -

Can you add these changes to the MySQL extension - this will allow us to keep MySQL, DBInterface, and CSV as weakdeps to keep the package light. You can still leave it as a separate file in the ext fold, but just need to include it in underneath

using TidierDB
using DataFrames
using MySQL
using CSV, DBInterface
__init__() = println("Extension was loaded!")
include("path.jl")

For copy_to2, it can also be named copy_to and with multiple dispatch. as long as the con is con::MySQL.Connection there should not be conflict issue. When you define it in the ext, you will just need to prefix it with TidierDB.

I have not tried running a docstring from an ext file. If it gives you issues, dont worry about it and i can try sorting it out/putting in a replace arg in the copy_to for main and just explaining there

thank you!

@vituri
Copy link
Contributor Author

vituri commented Oct 10, 2024

I added the code to the ext as recommended; I have no idea how to test it locally. I guess it is time to learn about extensions...

@drizk1
Copy link
Member

drizk1 commented Oct 10, 2024

Ok this is near perfect.. last thing (im sorry since I dont actually know how to take over a PR yet and easily edit it)

can you change the argument order in copy_to to the following so the connection comes firs so that it matches the others?
function copy_to2(con::MySQL.Connection, df::DataFrames.AbstractDataFrame, table_name; replace = false)

as for the deps, it looks like MySQL, CSV, DBInterface are still in [deps] rather than [weakdeps] ? would u mind moving those to the weakdeps?

and then ill merge and release asap.. just finishing up one other refinement to allow multiple TidierDB queries to be joined in a single chain

As far as local testing goes.. I too am not sure how to best do this for non duckdb backends. I've written about 60 TidierDB to tidierdata duckdb backend tests but I'm not sure how to best blanket those to the other 10 backends

EDIT
For testing the package locally, you run the tests from the run test file by hitting the play. It tests for a minute, then you just use the repl. So you would write in using MySQL to then test the copy_to function

@vituri
Copy link
Contributor Author

vituri commented Oct 11, 2024

No problem! See if it's ok now.

@drizk1
Copy link
Member

drizk1 commented Oct 11, 2024

Looks. Good last thing is like 54 end for the end of the module is now in the middle of the MySQL ext. So that end just needs to be moved to the last line.

@vituri
Copy link
Contributor Author

vituri commented Oct 13, 2024

Ops! The "end" got into the mess. Fixed.

@drizk1 drizk1 merged commit cb5836b into TidierOrg:main Oct 13, 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.

2 participants