BUG: to_sql with datetime.time values with sqlite #11547

nbonnotte · 2015-11-08T00:06:08Z

I'm proposing a solution to #8341

As @jorisvandenbossche suggested, I use a sqlite3 adapter to transform datetime.time objects into string (hh:mm:ss.ffffff, as this what sqlalchemy... I think?)

I added a test in the class _TestSQLApi, so that the solution is tested with both sqlalchemy and the sqlite3 fallback. Thus, the result should be consistent.

jorisvandenbossche · 2015-11-08T22:56:25Z

@nbonnotte Thanks for working on this!

As a quick feedback: I think we will also want this to work in the other direction, so from sql -> python, that you get back datetime.time instances (so not only a 'adapter' but also 'converter' in sqlite terminology, https://docs.python.org/2/library/sqlite3.html#converting-sqlite-values-to-custom-python-types)

nbonnotte · 2015-11-18T13:21:04Z

A quick update

@jorisvandenbossche to go the other way around is a bit more tricky, because (if i'm not mistaken) we depend on the sqlite3 connexion provided by the user.

Quick recap: in order for sqlite3 to know how to convert data back into the proper Python object, the user need to provide a parameter detect_types to sqlite3.connect specifying how to do that. There are 3 possibilities:

The value sqlite3.PARSE_DECLTYPES tells sqlite3 to "parse" (how exactly?) the declared sql type
The value sqlite3.PARSE_COLNAMES tells it to use the column name
Not providing any value tells sqlite3 not to convert the data and let it as a string

Since the dataframe we want to sqlize most likely has names for its columns, I guess we can exclude option 2. For backward compatibility reason, option 3 needs to be kept working, and option 1 is the one we want to implement here.

From a very practical point of view, we thus need to have tests for both option 1 and option 3, because we don't know the connection the user will throw at us.

That's when some difficulties arise: in pandas/io/test_sql.py, just adding the option detect_types=sqlite3.PARSE_DECLTYPES in all sqlite3.connect without specifying any convertors causes errors for time and date objects. One is a bit surprising:

  File "/home/nicolas/Git/pandas/pandas/io/tests/test_sql.py", line 1923, in test_datetime_date
    tm.assert_frame_equal(res, df.astype(str))
...
DataFrame.iloc[:, 0] values are different (100.0 %)
[left]:  [2014-01-01, 2014-01-02]
[right]: [2014-01-01, 2014-01-02]

I hope it's just a matter of type conversion, that for some strange reason looks really really weird.

I haven't had much time to look precisely onto it yet, but going the other way around (sqlite → python) will definitely demand more work than my initial pull request (python → sqlite).

nbonnotte · 2015-11-19T10:21:15Z

@jorisvandenbossche So basically, with the current pull request, the sqlite3 fallback should work exactly as with sqlalchemy.

To go further, that is to also provide a converter, would cause the "sqlite3 fallback" to have more features than with sqlalchemy, because then you would be able with the fallback to end up with, say, datetime objects, but not with sqlalchemy.

Am I wrong?

And assuming it is still an interesting direction to go, wouldn't it be better to have a new, separate issue, and let this PR close #8341?

jreback · 2016-01-06T17:16:56Z

@jorisvandenbossche status of this?

jreback · 2016-01-20T14:16:04Z

status of this?

nbonnotte · 2016-02-03T10:15:18Z

@jorisvandenbossche Maybe we could let this PR close issue #8341, and start a new issue to go in the other direction (sql -> python) ? What do you think?

max-sixty · 2016-02-03T15:53:35Z

pandas/io/sql.py

+        # this will transform time(12,34,56,789) into '12:34:56.000789'
+        # (this is what sqlalchemy does)
+        sqlite3.register_adapter(time, lambda _: _.strftime("%H:%M:%S.%f"))
+        super(self.__class__, self).__init__(*args, **kwargs)


Will this work correctly if inherited? You generally want super(SQLiteTable, self).__init__(*args, **kwargs)

Oh yeah, thanks

jorisvandenbossche · 2016-02-04T12:42:23Z

@nbonnotte yes, certainly a good idea to limit it here to pandas->sql!

nbonnotte · 2016-02-04T12:43:08Z

Great. And it's all green! \o/

jorisvandenbossche · 2016-02-04T12:43:41Z

pandas/io/sql.py

+    def __init__(self, *args, **kwargs):
+        # GH 8341
+        # register an adapter callable for datetime.time object
+        # if we're here, it means we have sqlite3


sqlite3 is standard library, so the comment can be removed I think.

You're right, thanks

nbonnotte · 2016-02-04T15:10:13Z

Pull request updated and green

jreback · 2016-02-08T15:40:50Z

@jorisvandenbossche ?

jreback · 2016-02-08T15:41:40Z

pandas/io/sql.py

+    def __init__(self, *args, **kwargs):
+        # GH 8341
+        # register an adapter callable for datetime.time object
+        import sqlite3


is this standard practice to do this in the init? (rather than at a class level)

I think the idea is to only have this adapter being registered if you actually use pandas with to_sql, not if you just import pandas to do other things (as this changes the behaviour of sqlite).

But actually, we should maybe test how this possibly interferes with sqlalchemy. @nbonnotte Can you test that writing/reading time values with sqlalchemy still works after having run this?

@nbonnotte did you for @jorisvandenbossche comments?

Not yet, I'm a bit busy right now, but I've noted :)

Done... I think, see the test below

@jorisvandenbossche is it what you had in mind?

jorisvandenbossche · 2016-02-23T13:03:58Z

pandas/io/tests/test_sql.py

-        if self.flavor == 'sqlite':
-            self.assertRaises(sqlite3.InterfaceError, sql.to_sql, df,
-                              'test_time', self.conn)
-


Can you leave this test here? (but the updated version of course with checking of the output)

jorisvandenbossche · 2016-02-23T13:10:13Z

@nbonnotte Sorry for the delay, but some comments on the tests

nbonnotte · 2016-02-24T09:46:56Z

I've updated the PR taking into account comments from @jorisvandenbossche

jorisvandenbossche · 2016-02-24T10:06:21Z

Thanks @nbonnotte
I you put the whatsnew in 0.18.0, I think this can still be merged

closes #8341

nbonnotte · 2016-02-24T12:13:19Z

Ok. If it can be shipped in 0.18.0, all the better — otherwise I'll change the whatsnew again.

All green.

jorisvandenbossche · 2016-02-24T12:54:28Z

@nbonnotte merged, thanks!

jorisvandenbossche added the IO SQL to_sql, read_sql, read_sql_query label Nov 8, 2015

max-sixty reviewed Feb 3, 2016
View reviewed changes

jorisvandenbossche reviewed Feb 4, 2016
View reviewed changes

jreback added this to the 0.18.0 milestone Feb 8, 2016

jreback reviewed Feb 8, 2016
View reviewed changes

jreback modified the milestones: 0.18.1, 0.18.0 Feb 12, 2016

jorisvandenbossche reviewed Feb 23, 2016
View reviewed changes

BUG: to_sql with datetime.time values with sqlite, #8341

8d449c5

closes #8341

jorisvandenbossche modified the milestones: 0.18.0, 0.18.1 Feb 24, 2016

jorisvandenbossche closed this in a652290 Feb 24, 2016

jorisvandenbossche mentioned this pull request Feb 24, 2016

BUG: to_sql fails with datetime.time values with sqlite fallback mode #8341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: to_sql with datetime.time values with sqlite #11547

BUG: to_sql with datetime.time values with sqlite #11547

nbonnotte commented Nov 8, 2015

jorisvandenbossche commented Nov 8, 2015

nbonnotte commented Nov 18, 2015

nbonnotte commented Nov 19, 2015

jreback commented Jan 6, 2016

jreback commented Jan 20, 2016

nbonnotte commented Feb 3, 2016

max-sixty Feb 3, 2016

nbonnotte Feb 4, 2016

jorisvandenbossche commented Feb 4, 2016

nbonnotte commented Feb 4, 2016

jorisvandenbossche Feb 4, 2016

nbonnotte Feb 4, 2016

nbonnotte commented Feb 4, 2016

jreback commented Feb 8, 2016

jreback Feb 8, 2016

jorisvandenbossche Feb 8, 2016

jreback Feb 12, 2016

nbonnotte Feb 12, 2016

nbonnotte Feb 17, 2016

jorisvandenbossche Feb 23, 2016

jorisvandenbossche commented Feb 23, 2016

nbonnotte commented Feb 24, 2016

jorisvandenbossche commented Feb 24, 2016

nbonnotte commented Feb 24, 2016

jorisvandenbossche commented Feb 24, 2016

BUG: to_sql with datetime.time values with sqlite #11547

BUG: to_sql with datetime.time values with sqlite #11547

Conversation

nbonnotte commented Nov 8, 2015

jorisvandenbossche commented Nov 8, 2015

nbonnotte commented Nov 18, 2015

nbonnotte commented Nov 19, 2015

jreback commented Jan 6, 2016

jreback commented Jan 20, 2016

nbonnotte commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Feb 4, 2016

nbonnotte commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbonnotte commented Feb 4, 2016

jreback commented Feb 8, 2016

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

jorisvandenbossche commented Feb 23, 2016

nbonnotte commented Feb 24, 2016

jorisvandenbossche commented Feb 24, 2016

nbonnotte commented Feb 24, 2016

jorisvandenbossche commented Feb 24, 2016