REGR: fix roundtripping datetimes with sqlite type detection #55690
Conversation
jorisvandenbossche
added
Regression
|
|
||
| sqlite3.register_adapter(time, _adapt_time) | ||
|
|
||
| sqlite3.register_adapter(date, adapt_date_iso) | ||
| sqlite3.register_adapter(datetime, adapt_datetime_iso) | ||
|
|
||
| convert_date = lambda val: date.fromisoformat(val.decode()) | ||
| convert_datetime = lambda val: datetime.fromisoformat(val.decode()) | ||
| convert_timestamp = lambda val: datetime.fromtimestamp(int(val)) | ||
| convert_timestamp = lambda val: datetime.fromisoformat(val.decode()) |
There was a problem hiding this comment.
@lithomas1 I think you added those from the documentation, but the actual implementation in cpython that we inherited is different: https://github.com/erlend-aasland/cpython/blob/07988dc731bc1e0f6ce2aac201d52e4b78618877/Lib/sqlite3/dbapi2.py#L71-L87
I didn't add that implementation exactly, as I think it should essentially be the same as parsing with fromisoformat (I suppose that didn't exist yet when this was originally implemented in cpython)
There was a problem hiding this comment.
Super sorry about this.
I just copied from https://docs.python.org/3/library/sqlite3.html#sqlite3-adapter-converter-recipes blindly when I was doing the changes for Python 3.12, and didn't pay super close attention since tests didn't fail.
Unfortunately, looks like my oversight bit us twice :( .
There was a problem hiding this comment.
No problem, not having test coverage doesn't help, and I have to say that the python docs are also quite misleading because it doesn't seem to work by copying verbatim
|
The changes LGTM assuming this is green. |
Co-authored-by: Matthew Roeschke <[email protected]>
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…dev#55690) Co-authored-by: Matthew Roeschke <[email protected]> (cherry picked from commit aeb3644)
|
Manual backport -> #55702 |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.#54985 was already a related regression fix in 2.1.1, but that only fixed the adapter (conversion to sqlite), not the converter (conversion back from sqlite to python). This converter only gets used when explicitly asking sqlite to detect dtypes.