ENH: to_sql() add parameter "method" to control insertions method (#8… #21401

schettino72 · 2018-06-09T11:01:04Z

…953)

closes Use multi-row inserts for massive speedups on to_sql over high latency connections #8953
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…ndas-dev#8953)

pep8speaks · 2018-06-09T11:01:11Z

Hello @schettino72! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 27, 2018 at 23:51 Hours UTC

schettino72 · 2018-06-09T11:05:16Z

Added docs
removed COPY implementation from code and added on docs

There is only one problem with flake8 complaining about a long.
The problem is that it is a url link, not sure how to break in into multiple lines...

schettino72 · 2018-06-09T11:07:44Z

Note: I am targeting this to 0.24, not 0.23.x.
IMO an addition of parameter should not be made a on bug release.
Otherwise code written for 0.32.2 using the new parameter would not run on 0.23.1...

gfyoung · 2018-06-10T04:59:09Z

pandas/core/generic.py

+
+                  sql = 'COPY {} ({}) FROM STDIN WITH CSV'.format(
+                      table_name, columns)
+                  cur.copy_expert(sql=sql, file=s_buf)


Nice explanation here! Do you mind moving some of it to the parameter description? That's probably where viewers will look first.

Yep, indeed (similar to my comment above), I would at least put a basic explanation of each option in the parameter description.
And then you can refer to below for more detailed performance considerations / example ..

And maybe we can put this example in the user guide (io.rst) ?

gfyoung · 2018-06-10T05:00:57Z

Note: I am targeting this to 0.24, not 0.23.x.

@schettino72 : That's exactly what we would do as well. Good call!

gfyoung · 2018-06-10T05:02:09Z

@schettino72 : Looks like you have a lint error according to Travis here.

jorisvandenbossche

Thanks for the PR. Added some comments, didn't look at the tests yet

pandas/core/generic.py

jorisvandenbossche · 2018-06-10T08:10:46Z

pandas/core/generic.py

+
+                  sql = 'COPY {} ({}) FROM STDIN WITH CSV'.format(
+                      table_name, columns)
+                  cur.copy_expert(sql=sql, file=s_buf)


Yep, indeed (similar to my comment above), I would at least put a basic explanation of each option in the parameter description.
And then you can refer to below for more detailed performance considerations / example ..

jorisvandenbossche · 2018-06-10T08:12:31Z

pandas/core/generic.py

+            This usually provides a big performance for Analytic databases
+            like *Presto* and *Redshit*, but has worse performance for
+            traditional SQL backend if the table contains many columns.
+            For more information check SQLAlchemy `documention <http://docs.sqlalchemy.org/en/latest/core/dml.html?highlight=multivalues#sqlalchemy.sql.expression.Insert.values.params.*args>`__.


If you start the url on the next line, I think flake8 will be OK with the too long line, like:

For more information check SQLAlchemy `documention <http://docs.sqlalchemy.org/en/latest/core/dml.html?highlight=multivalues#sqlalchemy.sql.expression.Insert.values.params.*args>`__.

jorisvandenbossche · 2018-06-10T08:13:10Z

pandas/core/generic.py

+
+         - `'default'`: Uses standard SQL `INSERT` clause
+         - `'multi'`: Pass multiple values in a single `INSERT` clause.
+            It uses a **special** SQL syntax not supported by all backends.


The 'It' should be indented equally to the `'multi'` of the line above (now it is one space more I think)

jorisvandenbossche · 2018-06-10T08:13:26Z

pandas/core/generic.py

+            traditional SQL backend if the table contains many columns.
+            For more information check SQLAlchemy `documention <http://docs.sqlalchemy.org/en/latest/core/dml.html?highlight=multivalues#sqlalchemy.sql.expression.Insert.values.params.*args>`__.
+         - callable: with signature `(pd_table, conn, keys, data_iter)`.
+            This can be used to implement more performant insertion based on


same here about the indentation

jorisvandenbossche · 2018-06-10T08:14:48Z

pandas/core/generic.py

+        The parameter ``method`` controls the SQL insertion clause used.
+        Possible values are:
+
+         - `'default'`: Uses standard SQL `INSERT` clause


Do we need to say this is a single insert statement for each row?

jorisvandenbossche · 2018-06-10T08:15:24Z

pandas/core/generic.py

+
+                  sql = 'COPY {} ({}) FROM STDIN WITH CSV'.format(
+                      table_name, columns)
+                  cur.copy_expert(sql=sql, file=s_buf)


And maybe we can put this example in the user guide (io.rst) ?

jorisvandenbossche · 2018-06-10T08:16:43Z

pandas/io/sql.py

+    def insert(self, chunksize=None, method=None):
+
+        # set insert method
+        if method in (None, 'default'):


is the 'None' needed here?

…andas-dev#8953)

codecov · 2018-06-10T17:12:11Z

Codecov Report

Merging #21401 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #21401   +/-   ##
=======================================
  Coverage   91.89%   91.89%           
=======================================
  Files         153      153           
  Lines       49596    49596           
=======================================
  Hits        45576    45576           
  Misses       4020     4020

Flag	Coverage Δ
#multiple	`90.29% <ø> (ø)`	⬆️
#single	`41.86% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/generic.py	`96.12% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 415012f...1e5d1cc. Read the comment docs.

codecov · 2018-06-10T17:12:13Z

Codecov Report

Merging #21401 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21401      +/-   ##
==========================================
+ Coverage   92.22%   92.22%   +<.01%     
==========================================
  Files         169      169              
  Lines       50897    50903       +6     
==========================================
+ Hits        46940    46946       +6     
  Misses       3957     3957

Flag	Coverage Δ
#multiple	`90.65% <ø> (ø)`	⬆️
#single	`42.29% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`96.84% <ø> (ø)`	⬆️
pandas/core/arrays/categorical.py	`95.56% <0%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a0c91e...f4ffbfc. Read the comment docs.

schettino72 · 2018-06-10T17:13:16Z

Thanks for quick review and tips. Ready for next round 😁

gfyoung · 2018-06-10T17:59:19Z

@schettino72 : Seems like you have test failures...

jreback · 2018-06-13T11:00:45Z

lgtm. @jorisvandenbossche merge when satisfied.

jreback · 2018-06-18T22:54:05Z

lgtm. ping @jorisvandenbossche

dahlbaek · 2018-08-01T11:35:06Z

doc/source/io.rst

+              table_name = table.name
+
+          sql = 'COPY {} ({}) FROM STDIN WITH CSV'.format(
+              table_name, columns)


I realise this is just an example, but isn't this piece vulnerable to SQL injection? thepsycopg2.sql module has an Identifier object designed to handle SQL identifiers appropriately. See also Example 41-1. Quoting Values In Dynamic Queries.

@dahlbaek no expert on this, but if you could provide the code that you would replace it with to make it more robust, that would certainly be welcome!

danfrankj · 2018-09-20T17:41:55Z

@schettino72 would love to see this this ship - think we'll get it into the next release?

TomAugspurger · 2018-10-22T20:55:06Z

What's the status on this?

@schettino72 there's a merge conflict with master, if you could merge master and push.

wxianxin · 2018-10-23T13:31:16Z

@schettino72 The magnificent you are the only hope for this unbelievable feature to be delivered at this wonderful release.

schettino72 · 2018-10-23T19:13:44Z

Sorry, I dont have the time now to work on this.

It seems the conflict is only on README file.
IMO should be resolved at the time the pull request is actually merged (by someone with write access), not something i can do.
Otherwise I might be playing catch up forever...

tgriek · 2018-12-17T12:14:14Z

Waiting for this! Thanks guys.

…m-8953

pandas/io/sql.py

mroeschke · 2018-12-26T20:25:26Z

@jreback Merged master, changed the method default to None and all green. Functionality looks good to me, but not sure if there's any doc followups that needs to be made.

jreback · 2018-12-27T00:28:46Z

doc/source/io.rst

+          s_buf = StringIO()
+          writer = csv.writer(s_buf)
+          writer.writerows(data_iter)
+          s_buf.seek(0)


I agree this needs testing.

mroeschke · 2018-12-27T23:22:55Z

@jreback Added a test for the example in io.rst and all green.

jreback

lgtm. minor comment on the whatsnew.

doc/source/whatsnew/v0.24.0.rst

pandas/io/sql.py

jreback · 2018-12-28T20:20:00Z

@TomAugspurger ?

TomAugspurger · 2018-12-28T20:23:03Z

+1 aside from the example. I'll push a commit updating it.

mroeschke · 2018-12-28T21:04:14Z

Thanks @jreback, @TomAugspurger.

And thank you @schettino72!

danfrankj · 2018-12-28T22:19:38Z

Thank you all! Looking forward to using this :)

maxgrenderjones · 2019-01-04T11:20:54Z

Hooray! Thanks :)

…ndas-dev#8… (pandas-dev#21401) * ENH: to_sql() add parameter "method" to control insertions method (pandas-dev#8953) * ENH: to_sql() add parameter "method". Fix docstrings (pandas-dev#8953) * ENH: to_sql() add parameter "method". Improve docs based on reviews (pandas-dev#8953) * ENH: to_sql() add parameter "method". Fix unit-test (pandas-dev#8953) * doc clean-up * additional doc clean-up * use dict(zip()) directly * clean up merge * default --> None * Remove stray default * Remove method kwarg * change default to None * test copy insert snippit * print debug * index=False * Add reference to documentation

ENH: to_sql() add parameter "method" to control insertions method (pa…

a77cdfd

…ndas-dev#8953)

schettino72 mentioned this pull request Jun 9, 2018

ENH: 'to_sql()' add param 'method' to control insert statement (#21103) #21199

Closed

5 tasks

ENH: to_sql() add parameter "method". Fix docstrings (pandas-dev#8953)

21e8c04

gfyoung added IO SQL to_sql, read_sql, read_sql_query Performance Memory or execution speed performance labels Jun 10, 2018

gfyoung reviewed Jun 10, 2018

View reviewed changes

jorisvandenbossche added the Enhancement label Jun 10, 2018

jorisvandenbossche added this to the 0.24.0 milestone Jun 10, 2018

jorisvandenbossche reviewed Jun 10, 2018

View reviewed changes

ENH: to_sql() add parameter "method". Improve docs based on reviews (p…

1e5d1cc

…andas-dev#8953)

ENH: to_sql() add parameter "method". Fix unit-test (pandas-dev#8953)

085313c

karldw mentioned this pull request Jun 22, 2018

Maybe document the postgres_copy speed difference? catalyst-cooperative/pudl#149

Closed

dahlbaek reviewed Aug 1, 2018

View reviewed changes

wxianxin mentioned this pull request Aug 22, 2018

I would love to snowflake to support supports_multivalues_insert snowflakedb/snowflake-sqlalchemy#55

Closed

TomAugspurger mentioned this pull request Oct 22, 2018

to_sql is very slow for sql server with sqlalchemy #23274

Closed

kjford mentioned this pull request Nov 26, 2018

Adding (Insert or update if key exists) option to .to_sql #14553

Closed

jreback removed this from the 0.24.0 milestone Dec 2, 2018

Matt Roeschke added 3 commits December 25, 2018 16:08

Merge remote-tracking branch 'upstream/master' into tosql-insert-para…

59cbdf7

…m-8953

use dict(zip()) directly

7bdf6f3

clean up merge

44f321a

jreback requested changes Dec 26, 2018

View reviewed changes

pandas/io/sql.py Outdated Show resolved Hide resolved

Matt Roeschke added 4 commits December 26, 2018 10:20

default --> None

d5ccabf

Remove stray default

643e9bf

Remove method kwarg

6fc6a26

change default to None

87730f3

jreback requested changes Dec 27, 2018

View reviewed changes

Matt Roeschke added 3 commits December 27, 2018 13:19

test copy insert snippit

f36710b

print debug

19f9dfa

index=False

c0bf457

jreback added this to the 0.24.0 milestone Dec 27, 2018

jreback requested changes Dec 27, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.rst Outdated Show resolved Hide resolved

pandas/io/sql.py Show resolved Hide resolved

Add reference to documentation

19ce379

jreback approved these changes Dec 27, 2018

View reviewed changes

mroeschke merged commit c1af4f5 into pandas-dev:master Dec 28, 2018

oguzhanogreden mentioned this pull request Jun 15, 2019

DOC: Reference to type 'pd_table' in IO Tools User Guide #26868

Closed

ENH: to_sql() add parameter "method" to control insertions method (#8… #21401

ENH: to_sql() add parameter "method" to control insertions method (#8… #21401

Conversation

schettino72 commented Jun 9, 2018 • edited

pep8speaks commented Jun 9, 2018 • edited

Comment last updated on December 27, 2018 at 23:51 Hours UTC

schettino72 commented Jun 9, 2018

schettino72 commented Jun 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung commented Jun 10, 2018

gfyoung commented Jun 10, 2018 • edited

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2018

Codecov Report

codecov bot commented Jun 10, 2018 • edited

Codecov Report

schettino72 commented Jun 10, 2018

gfyoung commented Jun 10, 2018

jreback commented Jun 13, 2018

jreback commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danfrankj commented Sep 20, 2018

TomAugspurger commented Oct 22, 2018

wxianxin commented Oct 23, 2018

schettino72 commented Oct 23, 2018

tgriek commented Dec 17, 2018

mroeschke commented Dec 26, 2018

Choose a reason for hiding this comment

mroeschke commented Dec 27, 2018

jreback left a comment

Choose a reason for hiding this comment

jreback commented Dec 28, 2018

TomAugspurger commented Dec 28, 2018

mroeschke commented Dec 28, 2018

danfrankj commented Dec 28, 2018

maxgrenderjones commented Jan 4, 2019

schettino72 commented Jun 9, 2018 •

edited

pep8speaks commented Jun 9, 2018 •

edited

gfyoung commented Jun 10, 2018 •

edited

codecov bot commented Jun 10, 2018 •

edited