Skip to content

[Python] pa.scalar(pd.Timedelta) with second or millisecond unit performs invalid conversion  #37291

Open

Description

@mroeschke

Describe the bug, including details regarding any error messages, version, and platform.

In [1]: import pandas as pd; import pyarrow as pa

In [9]: pa.scalar(pd.Timedelta(days=1).as_unit("s"))
Out[9]: <pyarrow.DurationScalar: datetime.timedelta(0)>

In [10]: pa.scalar(pd.Timedelta(days=1).as_unit("ms"))
Out[10]: <pyarrow.DurationScalar: datetime.timedelta(0)>

In [11]: pa.scalar(pd.Timedelta(days=1).as_unit("us"))
Out[11]: <pyarrow.DurationScalar: datetime.timedelta(days=1)>

In [12]: pa.scalar(pd.Timedelta(days=1).as_unit("ns"))
Out[12]: <pyarrow.DurationScalar: datetime.timedelta(days=1)>

In [13]: pa.__version__
Out[13]: '12.0.0'  # same result with 13.0.0.dev715

I would expect all 4 results to return datetime.timedelta(days=1)

Component(s)

Python

Activity

  1. jorisvandenbossche commented on Aug 22, 2023

    @jorisvandenbossche
    Member

    Thanks for the report @mroeschke. Looking into it, this might actually be a bug in pandas.

    In all cases, we currently interpret this pd.Timedelta object as a datetime.timedelta in the type inference (so we currently actually ignore the nano part if you don't manually specify that as the data type; this is a known limitation, eg #18099 and #36035 for the equivalent Timestamp case).

    But when putting a breakpoint in our conversion utility, it seems that when I pass a nanosecond-resolution Timedelta we properly get the number of days, but when it is a new seconds-resolution Timedelta, those attributes are not set:

    Running arr = pa.array([pd.Timedelta(days=1)]):

    Thread 1 "python" hit Breakpoint 2, arrow::py::internal::PyDelta_to_s (pytimedelta=0x7fffd75d5120) at /home/joris/scipy/repos/arrow/python/pyarrow/src/arrow/python/datetime.h:149
    149	  return (PyDateTime_DELTA_GET_DAYS(pytimedelta) * 86400LL +
    (gdb) p pytimedelta->seconds
    $2 = 0
    (gdb) p pytimedelta->microseconds
    $3 = 0
    (gdb) p pytimedelta->days
    $4 = 1                             # <---- properly set to 1
    

    Running arr = pa.array([pd.Timedelta(days=1).as_unit('s')]):

    Thread 1 "python" hit Breakpoint 1, arrow::py::internal::PyDelta_to_s (pytimedelta=0x7fffd75d51c0) at /home/joris/scipy/repos/arrow/python/pyarrow/src/arrow/python/datetime.h:149
    149	  return (PyDateTime_DELTA_GET_DAYS(pytimedelta) * 86400LL +
    (gdb) p pytimedelta->seconds
    $1 = 0
    (gdb) p pytimedelta->microseconds
    $2 = 0
    (gdb) p pytimedelta->days
    $3 = 0                             # <---- this is now 0 !
    
    
  2. jorisvandenbossche commented on Aug 22, 2023

    @jorisvandenbossche
    Member

    So this is actually documented in the pandas' source code:

    In the Timedelta constructor (https://github.com/pandas-dev/pandas/blob/43691a2f5d235b08f0f3aa813d8fdcb7c4ce1e47/pandas/_libs/tslibs/timedeltas.pyx#L953-L964):

        # For millisecond and second resos, we cannot actually pass int(value) because
        #  many cases would fall outside of the pytimedelta implementation bounds.
        #  We pass 0 instead, and override seconds, microseconds, days.
        #  In principle we could pass 0 for ns and us too.
        if reso == NPY_FR_ns:
            td_base = _Timedelta.__new__(cls, microseconds=int(value) // 1000)
        elif reso == NPY_DATETIMEUNIT.NPY_FR_us:
            td_base = _Timedelta.__new__(cls, microseconds=int(value))
        elif reso == NPY_DATETIMEUNIT.NPY_FR_ms:
            td_base = _Timedelta.__new__(cls, milliseconds=0)
        elif reso == NPY_DATETIMEUNIT.NPY_FR_s:
            td_base = _Timedelta.__new__(cls, seconds=0)
    

    and then the relevant note in the seconds attribute (https://github.com/pandas-dev/pandas/blob/43691a2f5d235b08f0f3aa813d8fdcb7c4ce1e47/pandas/_libs/tslibs/timedeltas.pyx#L1103-L1104):

            # NB: using the python C-API PyDateTime_DELTA_GET_SECONDS will fail
            #  (or be incorrect)
    

    So pyarrow is exactly using those C APIs, which are now silently incorrect for second and millisecond resolution Timedeltas.

    While pyarrow should certainly adapt to properly support pandas.Timedelta (especially for cases outside the range of datetime.timedelta), I think it might be safer for pandas to ensure those C APIs keep working (I think pyarrow will certainly not be the only one using those).
    It seems possible to me to at least set those for the cases where they are within the range of datetime.timedelta.

  3. jorisvandenbossche commented on Aug 22, 2023

    @jorisvandenbossche
    Member

    Opened pandas-dev/pandas#54682 on the pandas side

  4. BenjaminHelyer commented on Nov 3, 2023

    @BenjaminHelyer

    (Posting here for reference.) From the Pandas side, it's been decided that we won't support the C APIs, since this isn't possible to support cleanly in the general case. So the fix needs to happen on the PyArrow side.

  5. added this to the 16.0.0 milestone on Jan 24, 2024
  6. modified the milestones: 17.0.0, 18.0.0 on Jun 28, 2024
  7. removed this from the 18.0.0 milestone on Oct 2, 2024
  8. thisisnic commented on Oct 26, 2026

    @thisisnic
    Member

    This might have since been resolved in pandas 3.0.0

  9. thisisnic commented on Nov 10, 2026

    @thisisnic
    Member

    I'm just going through older bug issues to see if we can close them out or we need further action.

    Still reproduces on 26.0.1 with pandas 2.3.3 (s and ms give timedelta(0)), but gives the correct result with pandas 3.1.1. #40620 and #49222 look like the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions