Inconsistency in datetime.utcfromtimestamp(Decimal)

[serhiy-storchaka]

BPO	23607
Nosy	@malemburg, @abalkin, @vstinner, @skrah, @serhiy-storchaka, @pganssle, @shlomoa, @remilapeyre
Files	issue23607_python.diff datetime_fromtimestamp_decimal.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = None
created_at = <Date 2015-03-08.10:35:01.714>
labels = ['3.8', 'type-bug', 'library']
title = 'Inconsistency in datetime.utcfromtimestamp(Decimal)'
updated_at = <Date 2019-02-27.17:14:23.100>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2019-02-27.17:14:23.100>
actor = 'remi.lapeyre'
assignee = 'serhiy.storchaka'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2015-03-08.10:35:01.714>
creator = 'serhiy.storchaka'
dependencies = []
files = ['38388', '38736']
hgrepos = []
issue_num = 23607
keywords = ['patch']
message_count = 11.0
messages = ['237527', '237541', '237801', '239593', '239595', '239597', '239608', '239613', '336769', '336771', '336772']
nosy_count = 10.0
nosy_names = ['lemburg', 'belopolsky', 'vstinner', 'SilentGhost', 'skrah', 'serhiy.storchaka', 'p-ganssle', 'anglister', 'remi.lapeyre', 'Steven Davidson']
pr_nums = []
priority = 'low'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23607'
versions = ['Python 3.8']

[serhiy-storchaka]

First, datetime.utcfromtimestamp() drops fractional part of Decimal argument:

>>> import datetime
>>> from decimal import Decimal as D
>>> datetime.datetime.utcfromtimestamp(1425808327.307651)
datetime.datetime(2015, 3, 8, 9, 52, 7, 307651)
>>> datetime.datetime.utcfromtimestamp(D(1425808327.307651))
datetime.datetime(2015, 3, 8, 9, 52, 7)

Second, it works only with C implementation. With Python implementation Decimals are not supported at all.

>>> del sys.modules['datetime']
>>> sys.modules['_datetime'] = None
>>> import datetime
>>> datetime.datetime.utcfromtimestamp(D(1425808327.307651))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.4/datetime.py", line 1374, in utcfromtimestamp
    t, frac = divmod(t, 1.0)
TypeError: unsupported operand type(s) for divmod(): 'decimal.Decimal' and 'float'

In 2.7 Decimals are accepted and are not truncated.

>>> import datetime
>>> from decimal import Decimal as D
>>> datetime.datetime.utcfromtimestamp(D(1425808327.307651))
datetime.datetime(2015, 3, 8, 9, 52, 7, 307651)

So this can be considered as a regression.

[SilentGhost]

Here is the python-only fix that eliminates TypeError and produces correct number of milliseconds. The problem with C implementation lies in _PyTime_ObjectToDenominator function where there is an explicit check for a float. This check could be relaxed to include decimals as the rest of the code seem to work fine with decimal. I'm not entirely sure how that could be done though.

[serhiy-storchaka]

The fix for Python implementation LGTM, but a fix for C implementation is needed too.

[serhiy-storchaka]

Here is a patch that adds Decimal support for datetime.fromtimestamp() and datetime.utcfromtimestamp(). As side effect Decimal timestamps now are supported in few other places, e.g. in os.utime().

[vstinner]

I reviewed datetime_fromtimestamp_decimal.patch.

As side effect Decimal timestamps now are supported in few other places, e.g. in os.utime().

It would be more consistent to support decimal.Decimal nowhere or "everywhere". IMO the new _PyTime_FromSecondsObject() (very close to _PyTime_ObjectToDenominator, but using time_t) should also be patched.

Please add some tests for decimal.Decimal in test_time directly. Usually, I try to test rounding and overflow. Testing for overflow is not always possible because it may depend on the platform.

--

See also the PEP-410: "PEP-410 - Use decimal.Decimal type for timestamps". The PEP was rejected. The compromise was a new timestamp format for os.utime(): a number of nanoseconds since the UNIX epoch (1970-01-01). I partially implemented a private API for this PEP in the issue bpo-22117: the new _PyTime_t is a number of timestamp using an arbitrary resolution (currently, it's a number of nanoseconds using a 64-bit signed integer type).

[skrah]

I just looked at this very briefly: Is the patch context insensitive?

IOW, do things still work if you change the thread-local context:

from decimal import *
c = getcontext()
c.prec = 1
c.Emax = 1
c.Emin = -1

[serhiy-storchaka]

The problem is that the Decimal *was* supported in 2.7. So this issue can be considered not as adding new feature, but as fixing a regression. But changes are too large for just bugfix.

It would be more consistent to support decimal.Decimal nowhere or "everywhere". IMO the new _PyTime_FromSecondsObject() (very close to _PyTime_ObjectToDenominator, but using time_t) should also be patched.

Will add Decimal support in all functions in Python/pytime.c that support floats.

Please add some tests for decimal.Decimal in test_time directly. Usually, I try to test rounding and overflow. Testing for overflow is not always possible because it may depend on the platform.

Will do.

Is the patch context insensitive?

No, the patch is context sensitive. I think the end user is responsible to set an appropriate context if it want to create a datetime from Decimal timestamp. At least until we add functions that return Decimal.

[skrah]

I think we should try to avoid depending on global state in
the stdlib, at least in new code. Also, if something is not
really a decimal computation, Decimal itself tries to ignore
the context (like Decimal.__repr__).

At least I would expect this datetime function to be context
independent.

[pganssle]

I'm not sure if either of these patches got merged, but at some point this was fixed:

    Python 3.7.2 (default, Feb  9 2019, 13:18:43) 
    [GCC 8.2.1 20181127] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from datetime import datetime
    >>> from decimal import Decimal
    >>> datetime.utcfromtimestamp(Decimal(123456.0))
    datetime.datetime(1970, 1, 2, 10, 17, 36)

I recommend that someone add some regression tests to this, then we can close the issue.

I'm also going to mark this as "easy" since adding tests for this particular example should be pretty simple.

[pganssle]

Oh actually that's my mistake. I can't reproduce the failure in the constructor in the Python version of the module, and also it seems to be fixed in the pure Python version as of at least 3.6:

Python 3.6.7 (default, Oct 25 2018, 16:11:17) 
[GCC 8.2.1 20180831] on linux
>>> import sys
>>> sys.modules['_datetime'] = None
>>> from decimal import Decimal as D
>>> from datetime import datetime
>>> datetime.utcfromtimestamp(D(123456.12345))
datetime.datetime(1970, 1, 2, 10, 17, 36, 123450)

But the truncation behavior is still present in the C version as of Python 3.8.0a1+:

Python 3.8.0a1+ (heads/master:3766f18, Feb 11 2019, 12:52:31) 
[GCC 8.2.1 20181127] on linux
>>> from datetime import datetime
>>> from decimal import Decimal as D
>>> datetime.utcfromtimestamp(D(123456.12345))
datetime.datetime(1970, 1, 2, 10, 17, 36)

I still think we need a test for the constructor behavior, but I'm going to remove "easy", since we still need to fix truncation in the C version.

[serhiy-storchaka]

No, the bug is not fixed, and this is not easy issue. You should use non-integer Decimals to reproduce it. In 3.8 this emits a deprecation warning:

>>> import datetime
>>> from decimal import Decimal as D
>>> datetime.datetime.utcfromtimestamp(D(1425808327.307651))
<stdin>:1: DeprecationWarning: an integer is required (got type decimal.Decimal).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
datetime.datetime(2015, 3, 8, 9, 52, 7)