BUG: generated timestamps deviate from expected values

[chalmerlowe]

Pandas version checks

• I have checked that this issue has not already been reported.

• I have confirmed this bug exists on the latest version of pandas.

• I have confirmed this bug DOES NOT exist on the main branch of pandas (1.4.3 at the time of creating this Issue).

Reproducible Example

import pandas as pd
import db_dtypes

value = "23:59:59.999999"
expected = datetime.time(23, 59, 59, 999999)

def test_time_parsing(value, expected):
    assert pandas.Series([value], dtype="dbtime")[0] == expected

# The results of the above assert are:

# assert datetime.time(23, 59, 58, 999271) == datetime.time(23, 59, 59, 999999)

Issue Description

Formatted time strings are incorrectly converted to datetime.time objects.

When running the prerelease tests for python-db-dtypes-pandas and checking for prerelease issues that might be associated with the pandas dev branches, we found identified that several python-db-dtypes-pandas tests failed depending on which nightly pandas branch we use.

In particular between these two commits:

• b74dc5c077 yields PASSING TESTS
• 87da50042c yields FAILING TESTS

Not sure yet if this is due to something that derives from the python-db-dtypes-pandas lib OR from pandas OR from one of pandas' dependencies.

NOTE: the reproducible example provided is just one of many tests in the test suite that fail with a wide variety of different times, etc.

Expected Behavior

The expected result of the reproducible example should be:

assert datetime.time(23, 59, 59, 999999) == datetime.time(23, 59, 59, 999999)

Installed Versions

For reference here are the installed libraries when we ran these tests. When running the failing tests, the only package that changed was the version of pandas. All other packages were the same:

python==3.10
# >>>>>>>>>>>>>>>>>>>>>>
asttokens==2.0.8
asyncmock==0.4.2
attrs==22.1.0
backcall==0.2.0
coverage==6.4.4
-e git+ssh://git@github.com/googleapis/python-db-dtypes-pandas.git@23d88d9e843e903a6e5a123123825de1bd88c722#egg=db_dtypes
decorator==5.1.1
executing==0.10.0
iniconfig==1.1.1
ipython==8.4.0
jedi==0.18.1
matplotlib-inline==0.1.6
mock==4.0.3
numpy==1.23.2
packaging==21.3
pandas==1.5.0.dev0+686.g5b76b77399  # A passing version of pandas
parso==0.8.3
pexpect==4.8.0
pickleshare==0.7.5
pluggy==1.0.0
prompt-toolkit==3.0.30
ptyprocess==0.7.0
pure-eval==0.2.2
py==1.11.0
pyarrow==9.0.0
Pygments==2.13.0
pyparsing==3.0.9
pytest==7.1.2
pytest-asyncio==0.19.0
pytest-cov==3.0.0
python-dateutil==2.8.2
pytz==2022.2.1
six==1.16.0
stack-data==0.4.0
tomli==2.0.1
traitlets==5.3.0
wcwidth==0.2.5

[phofl]

Hi, thanks for your report. Could you run a git bisect to narrow it down further?

Small note: I have confirmed this bug DOES NOT exist on the main branch of pandas references our mainbranch, not a release version. But can confirm that it fails on main too.

I am not sure that this is on our side. We are passing ['2:22:21.222222', '2:22:23.222222'] to dbtype and we are getting back: <TimeArray> [datetime.time(2, 22, 20, 998883), datetime.time(2, 22, 22, 998883)] Length: 2, dtype: dbtime

So iiuc this is already wrong?

[chalmerlowe]

I ran git bisect. I updated the main description to highlight the two commits that git bisect was able to identify where we go from passing to failing tests.

I haven't been able to dig deeply enough into the code to identify exactly where things are going wrong, so any help would be appreciated.

[phofl]

Sorry did not see initially that the new commit is in June too

Could you please answer my other question? This looks to me as the issue is not on our side

[tswast]

@phofl @chalmerlowe I'm able to reproduce this without the db-dtypes package.

In [14]: s = pd.Series([pd.Timestamp(
    year=1970, month=1, day=1, hour=23, minute=59, second=59, nanosecond=999_999_000
)], dtype='datetime64[ns]')

In [15]: s
Out[15]: 
0   1970-01-01 23:59:58.999271621
dtype: datetime64[ns]

I believe the problem might lie here:

pandas/pandas/_libs/tslibs/conversion.pyx

Line 391 in e94faa2

obj.value -= int(offset.total_seconds() * pps)

As the call to total_seconds() can introduce floating point precision errors.

Edit: I see that total_seconds() logic was there before, but it seems something in that commit is resulting in a loss of precision.

[phofl]

Thank you very much.

cc @jbrockmendel any idea here?

[mroeschke]

From a quick look, it appears that the nanoseconds is derived from npy_datetimestruct.ps (picoseconds) and is assumed to fit into int32, but 999_999_000 * 1000 will overflow the max value of int32

[jbrockmendel]

@mroeschke where are we doing this multiplication?

[mroeschke]

IIUC here

pandas/pandas/_libs/tslibs/conversion.pyx

Line 384 in c08c925

obj.dts.ps = nanos * 1000

[mroeschke]

Some other possibly problematic places where we might be overflowing:

pandas/pandas/_libs/tslibs/timestamps.pyx

Line 2291 in e94faa2

dts.ps = self.nanosecond * 1000

pandas/pandas/_libs/tslibs/timestamps.pyx

Line 2317 in e94faa2

dts.ps = validate('nanosecond', nanosecond) * 1000

With this diff, I get the above example to work correctly again, but it feels like a band-aid fix:

--- a/pandas/_libs/tslibs/conversion.pyx
+++ b/pandas/_libs/tslibs/conversion.pyx
@@ -378,10 +378,14 @@ cdef _TSObject convert_datetime_to_tsobject(
         obj.tzinfo = ts.tzinfo

     if isinstance(ts, ABCTimestamp):
-        obj.dts.ps = ts.nanosecond * 1000
+        us, rem = divmod(ts.nanosecond, 1000)
+        obj.dts.us += us
+        obj.dts.ps += rem * 1000

     if nanos:
-        obj.dts.ps = nanos * 1000
+        us, rem = divmod(nanos, 1000)
+        obj.dts.us += us
+        obj.dts.ps += rem * 1000

     obj.value = npy_datetimestruct_to_datetime(reso, &obj.dts)

In [1]: In [14]: s = pd.Series([pd.Timestamp(
   ...:     year=1970, month=1, day=1, hour=23, minute=59, second=59, nanosecond=999_999_000
   ...: )], dtype='datetime64[ns]')

In [2]: s
Out[2]:
0   1970-01-01 23:59:59.999999
dtype: datetime64[ns]

Would be more ideal if we use a npy_datetimestruct that supported int_64 components. Thoughts cc @jbrockmendel

[jbrockmendel]

would it be easier to do (<int64_t>nanos) * 1000?

[mroeschke]

would it be easier to do (<int64_t>nanos) * 1000?

Tried this locally, but it didn't fix the issue. I think this is because dts.ps is still typed as int_32

https://github.com/numpy/numpy/blob/b65f0b7b8ba7e80b65773e06aae22a8369678868/numpy/core/include/numpy/ndarraytypes.h#L802

[mroeschke]

Playing around with the fix in #48255 (comment), I don't think this is a great solution either in the long run as spilling some of the nanoseconds into the microseconds could introduce further bugs like adding too many additional microseconds beyond the allowed cap (1000000).

I think to avoid this all together, no more than 999 nanoseconds should be allowed in the Timedelta constructor to mirror the behavior of other components in the datetime constructor. #48538

[tswast]

I think to avoid this all together, no more than 999 nanoseconds should be allowed in the Timedelta constructor to mirror the behavior of other components in the datetime constructor. #48538

A little unfortunate, but easy enough to workaround in googleapis/python-db-dtypes-pandas#148

If we can make this an error in 1.5 instead of an overflow, that would save a lot of debugging time for other folks who might hit this issue.