[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Xiang Zhang, was there a discussion on Python-Dev?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Hassanbot]

Hassanbot  added the comment:

This still isn't fixed as of 3.8 (or in master I think).

I can understand why you wouldn't want to allow serializing and deserializing 
time zones, since tzinfo objects cannot be accurately serialized with a simple 
UTC offset, but you should at least get an error when trying to insert an aware 
object. Anything is better than it is now, where you get no warning or error 
when inserting the object, and get a hard to interpret error ("invalid literal 
for int() with base 10") when trying to retrieve it.

For deserialization, the datetime class now (since 3.7) includes a 
fromisoformat() method that could be used as a counterpart to the isoformat() 
method used when serializing. At least it would be consistent then.

--
nosy: +hassanbot
versions: +Python 3.8 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

> I'm even worse at that. :-(

LoL. Okay I'll do that. :-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I'm even worse at that. :-(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

> Maybe discuss this on Python-Dev?

It's fine. Could you compose a mail instead of me? I am not good at that. :-(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Added few minor comments on Rietveld. Technically the patch LGTM. But the 
behavior change of the timestamp converter for input without timezone offset 
perhaps needs wider discussion. We have several options:

0. Current behavior. Silently drop the timezone if there are microseconds, and 
raise an error when there are no microseconds. This is bad for many reasons, 
but some user code can depend on this.

1. Just drop the timezone, don't raise an error when there are no microseconds. 
The converter will become returning incorrect result instead of just failing on 
unexpected input. This will fix the user code that is aware of this, but 
currently fails when input doesn't have microseconds.

2. Return aware datetime for input with timezone offset (as in original patch). 
Returning aware or naive datetime depending on input can break a user code that 
is not prepared for this.

3. Raise an error for input with timezone offset, always return naive datetime 
objects from this converter. This can break the user code that depends on 
current behavior and works with the input containing the same timezone offsets.

Any option can break some code. I prefer option 3, but want to hear thoughts of 
other core developers. Maybe discuss this on Python-Dev?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

Ping for review for timestamptz-3.patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

timestamptz-3.patch uses ValueError instead of ProgrammingError and treat 
suffix Z as UTC timezone.

--
Added file: http://bugs.python.org/file46279/timestamptz-3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

I am okay with ValueError（actually I use it in the patch originally) but I am 
not in favour of catching the errors of the parser. The parser raises errors 
due to invalid data, not timezone. I think propagate it to users could make the 
reason more obvious.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Perhaps even TypeError should be converted to ValueError.

But this is different issue of course.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I think ProgrammingError is not correct type of an exception. It is used for 
programming errors such as using uninitialized or closed objects or supplying 
incorrect number of bindings. But this error can be caused by invalid data in a 
database created not by Python. Currently converters raise ValueError or 
TypeError for invalid data. ValueError is raised for datetime without 
microseconds but with timezone offset. I think ValueError is more appropriate 
type for such errors.

Perhaps even TypeError should be converted to ValueError.

try:
# parsing date or datetime
except (ValueError, TypeError):
raise ValueError(...)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

timestamptz-2.patch make timestamp and timestamptz specilized for their own 
objects.

--
Added file: http://bugs.python.org/file46277/timestamptz-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

> I think the old code unlikely will be broken if preserve an exception type 
> and don't change conditions for raising an error.

Currently there could be no error when the object gets a timezone. The timezone 
is simply discarded and only when microseconds comes to 0 there is a 
ValueError. I don't think the user code would prepare to catch the ValueError. 
I don't oppose make timestamp more strict but just not sure if it's suitable.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Naive and aware objects should not be mixed. Their actually are different 
types. The converter doesn't return str or bytes when can't parse an input as a 
datetime, and it shouldn't return a naive datetime if can't find a timezone.

SQLite date and time functions imply UTC if timezone is omitted. This is a 
reason for returning aware UTC datetime objects. On other side, Python is more 
powerful programming language, it distinguish naive and aware datetime objects, 
and it is unlikely that these two types are mixed in one database column. It is 
better to raise an error that silently return possible wrong result. It 
exceptional case user can write special converter or just call SQLite 
datetime() for unifying data format.

I think the old code unlikely will be broken if preserve an exception type and 
don't change conditions for raising an error. New error message can contain 
full input string and suggest to use the timestamptz converter if it looks as a 
datetime with timezone.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

> I think the timestamptz converter should either interpret strings without 
> timezone as UTC (and perhaps understand the "Z" suffix as sqlite3 date() 
> function) or raises an error. It should never return naive datetime.

Currently timestamptz just convert back what the user passed in, no matter 
naive or aware objects. What to do with them is left to the app. If we raise an 
error, could users use naive and aware objects together? And interpreting 
strings without timezone as UTC seems will mistranslate the object. For 
example, pass in datetime.datetime.now() and translate it back as UTC may not 
be right.

> The timestamp converter needs better error reporting when get an input with a 
> timezone.

I thought about it but our intention to introduce a new converter is not to 
break old code. Doesn't add error reporting violate the intention? Users' code 
may not catch the error now.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I think the timestamptz converter should either interpret strings without 
timezone as UTC (and perhaps understand the "Z" suffix as sqlite3 date() 
function) or raises an error. It should never return naive datetime.

The timestamp converter needs better error reporting when get an input with a 
timezone.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

timestamptz.patch implements a new converter that's able to convert aware 
datetime objects.

--
stage: needs patch -> patch review
Added file: http://bugs.python.org/file46265/timestamptz.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

On 11.01.2017 17:04, Xiang Zhang wrote:
> I am not sure it's worth to make it even optional in 3.7. Discarding tzinfo 
> sounds weird, and worse, the behaviour is not deterministic, it could also 
> fail with an exception.

Best practice is to store all date/time values using UTC (or some
other fixed timezone) in databases and to manage the timezone/locale
information elsewhere.

The TZ info then becomes redundant and only results in more
database space being used as well as slower queries (due to the extra
TZ calculations being done; many databases internally store the
values as UTC to avoid having to do TZ calculations at query time).

Of course, there are use cases, where you'd still want to work
with TZ values in the database, so an extra converter for this
sounds like a good plan.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
stage: commit review -> needs patch
type: behavior -> enhancement
versions:  -Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

On 11.01.2017 17:08, Serhiy Storchaka wrote:
> 
> The naive datetime converter is registered under the name "timestamp". The 
> aware datetime converter or the universal datetime converter (if it is 
> needed) can be registered under different names.

This sounds like a good idea.

Perhaps use "timestamptz" or something similar.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

The naive datetime converter is registered under the name "timestamp". The 
aware datetime converter or the universal datetime converter (if it is needed) 
can be registered under different names.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

I asked whether this should be treated as an enhancement or bug fix. Now with 
your concerns I think it fits as enhancement more.

> Is it possible to make the support optional for 3.5 and 3.6 and only enable 
> it for 3.7 (with the possibility of disabling it again) ?

How about just make it just into 3.7 and just document only naive datetime 
objects are supported in 3.5, 3.6 and 2.7.
I am not sure it's worth to make it even optional in 3.7. Discarding tzinfo 
sounds weird, and worse, the behaviour is not deterministic, it could also fail 
with an exception.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

I don't think this can be considered a bug fix, since it changes behavior for 
applications that read data from SQLite databases which were not created by 
Python.

Those application may now see datetime values with tz infos and will likely not 
be prepared to handle all the problems associated with this.

Is it possible to make the support optional for 3.5 and 3.6 and only enable it 
for 3.7 (with the possibility of disabling it again) ?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Agreed. And maybe raise more informative error for datetimes with a timezone.

I'm also not sure about 3.5 and 3.6. Datetimes with timezones were never 
supported. Currently all returned ditetime objects are naive. Getting an aware 
datetime can confuse applications.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

> LGTM except that arguments of assertEqual() should be swapped.

Thanks Serhiy! I'll take care of that. One problem remained is what to do with 
2.7. There is no timezone object in 2.7. My preference is just pointing out the 
limitation that only naive datetime object is supported in the doc.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

LGTM except that arguments of assertEqual() should be swapped.

--
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Changes by Xiang Zhang :


Added file: http://bugs.python.org/file46254/sqlite3-3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

LGTM generally. :-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

In general the patch LGTM. Added comments on Rietveld.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
stage: test needed -> patch review
Added file: http://bugs.python.org/file46082/sqlite3-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Bozo Kopic]

Changes by Bozo Kopic :


Added file: http://bugs.python.org/file46081/sqlite3-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Bozo Kopic]

Changes by Bozo Kopic :


Removed file: http://bugs.python.org/file46075/sqlite3-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Bozo Kopic]

Changes by Bozo Kopic :


Added file: http://bugs.python.org/file46075/sqlite3-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Bozo Kopic]

Changes by Bozo Kopic :


Removed file: http://bugs.python.org/file46074/sqlite3-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Bozo Kopic]

Bozo Kopic added the comment:

I'm providing new patch that adds support for parsing timezone information.

Tests are included.

--
Added file: http://bugs.python.org/file46074/sqlite3-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

import sqlite3, datetime
c = sqlite3.connect(':memory:', 
detect_types=sqlite3.PARSE_DECLTYPES|sqlite3.PARSE_COLNAMES)
cur = c.cursor()
cur.execute('create table test(t timestamp)')
t = datetime.datetime.now(tz=datetime.timezone.utc)
cur.execute("insert into test(t) values (?)", (t,))
cur.execute('select t from test')
l = cur.fetchone()[0]
t == l   # the result not equal to the original one

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Could you provide a unittest?

--
nosy: +belopolsky, ghaering, lemburg
stage:  -> test needed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

I would prefer add the support for timezone since the current behaviour seems 
not correct to me. If you'd like to work on it, don't forget to add a test case 
and sign the CLA.

But I am not sure this should be treated as a bug or new feature.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Bozo Kopic]

Bozo Kopic added the comment:

Yes, that is correct. Timezone information is discarded. Submitted patch only 
resolves occurrence of ValueError.

Is additional patch that enables parsing of timezone required?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29099] sqlite3 timestamp converter cannot handle timezone

[Xiang Zhang]

Xiang Zhang added the comment:

Thanks for your report Bozo.

I think this is due to timestamp converter doesn't handle timezone now. Besides 
this ValueError, if I understand correctly, even if you pass a datetime object 
with tzinfo, with microsecond set, the returned datetime object is a wrong one 
since the tzinfo is discarded.

--
nosy: +xiang.zhang
title: sqlite3 timestamp converter ValueError -> sqlite3 timestamp converter 
cannot handle timezone
type: crash -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com