-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Truncate table option for to sql #50088
Truncate table option for to sql #50088
Conversation
pandas/io/sql.py
Outdated
self.meta.reflect(bind=self.con, only=[table_name], schema=schema) | ||
if schema: | ||
schema = schema + "." | ||
self.execute(f"DELETE FROM {schema or ''}{table_name}") |
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.
Shouldn't this just be TRUNCATE?
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 used DELETE since sqlite doesn't support TRUNCATE. However, since I had the if statement already for the schema, it will now use DELETE for the schema-less sqlite and use the faster TRUNCATE for everything else.
doc/source/whatsnew/v1.5.3.rst
Outdated
@@ -32,7 +32,7 @@ Bug fixes | |||
|
|||
Other | |||
~~~~~ | |||
- | |||
- Added ``"truncate"`` option to ``if_exists`` argument in :meth:`DataFrame.to_sql` to truncate the existing table (:issue:`37210`) |
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.
Can you move this to the 2.0 whatsnew?
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.
Done! Thanks for all your help with my first pandas PR!
@@ -1833,6 +1837,15 @@ def drop_table(self, table_name: str, schema: str | None = None) -> None: | |||
self.get_table(table_name, schema).drop(bind=self.con) | |||
self.meta.clear() | |||
|
|||
def trunc_table(self, table_name: str, schema: str | None = None) -> None: |
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.
What happens when you try to truncate a table that doesn't exist? Should this raise? If so can you add a test for that?
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.
If the table doesn't exist, it should just create a new table - added a test for it.
Also, added a test for if truncate is selected and then new columns are designated to write to the table. This should throw an error on the database.
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 would rather we raise here if the DB doesn't support truncate. In the future there could be a use for a delete_from
argument in addition to truncate
, so merging the two here dependent on the DB is confusing
@@ -1833,6 +1837,15 @@ def drop_table(self, table_name: str, schema: str | None = None) -> None: | |||
self.get_table(table_name, schema).drop(bind=self.con) | |||
self.meta.clear() | |||
|
|||
def trunc_table(self, table_name: str, schema: str | None = None) -> None: |
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 would rather we raise here if the DB doesn't support truncate. In the future there could be a use for a delete_from
argument in addition to truncate
, so merging the two here dependent on the DB is confusing
@@ -2253,6 +2265,10 @@ def drop_table(self, name: str, schema: str | None = None) -> None: | |||
drop_sql = f"DROP TABLE {_get_valid_sqlite_name(name)}" | |||
self.execute(drop_sql) | |||
|
|||
def trunc_table(self, name: str, schema: str | None = None) -> None: |
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 think this method should be deleted, or should explicitly raise a NotImplementedError
for sqlite. Can you also set up a test called test_sqlite_truncate_raises
that makes sure that happens? You'll see that pattern in many of the other tests
|
||
def test_to_sql_truncate_no_table(self, test_frame1): | ||
# creates new table if table doesn't exist | ||
sql.to_sql(test_frame1, "test_frame_new", self.conn, if_exists="truncate") |
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.
Think this should raise too? Feels a little strange to have truncate create a table
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.
Though I guess this is consistent with replace
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
What else needs to be done to get truncate into pandas ? |
Not 100% sure - this is my first ever PR attempt for an open source project. I think the code is good to go but let it go stale (apologies) when things got busy. At the very least I'll need to solve for some conflicts and update the I'll start working on it again but if you're willing, I could certainly use some help/guidance in getting it over the finish line. |
doc/source/whatsnew/v1.5.3.rst
file if fixing a bug or adding a new feature.