[C++][Python] Incorrect result for `floor_temporal` with 3 and 'year'

[MarcoGorelli]

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

import pyarrow as pa
import pyarrow.compute as pc
import duckdb

print(duckdb.sql(
    """
    from values (timestamp '1970-01-01') df(a)
    select time_bucket('3 years', "a", timestamp '1970-01-01')
    """
))
print(pc.floor_temporal(pa.array([datetime(1970, 1, 1)]), 3, 'year'))

Outputs:

┌────────────────────────────────────────────────────────────┐
│ time_bucket('3 years', a, CAST('1970-01-01' AS TIMESTAMP)) │
│                         timestamp                          │
├────────────────────────────────────────────────────────────┤
│ 1970-01-01 00:00:00                                        │
└────────────────────────────────────────────────────────────┘

[
  1968-01-01 00:00:00.000000
]

The DuckDB output differs from the PyArrow one. Given that the pyarrow docs say

By default, the origin is 1970-01-01T00:00:00.

I would expect it to be aligned with DuckDB when specifying timestamp '1970-01-01' as origin.

In fact, if I use 36, 'month', then PyArrow also returns '1970-01-01'. The fact that 3, 'year' differs from 3*12, 'month' suggests to me that there's a bug

In [6]: pc.floor_temporal(arr, 3, 'year')
Out[6]: 
<pyarrow.lib.TimestampArray object at 0x7fe44180fca0>
[
  1968-01-01 00:00:00.000000
]

In [7]: pc.floor_temporal(arr, 3*12, 'month')
Out[7]: 
<pyarrow.lib.TimestampArray object at 0x7fe443a39540>
[
  1970-01-01 00:00:00.000000
]

Component(s)

Python

[AlenkaF]

I can confirm this behaviour (it seems not all unit options are tested in Python either:)

arrow/python/pyarrow/tests/test_compute.py

Lines 2563 to 2564 in 53ef438

	@pytest.mark.parametrize('unit', ("nanosecond", "microsecond", "millisecond",
	"second", "minute", "hour", "day"))

@rok I suspect this edge case was not thought of? (looking at the C++ tests it seems all units are included but not with this value case)

[rok]

Indeed pyarrow curently considers year 0 as origin for rounding years:

arrow/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Lines 1041 to 1047 in 067fd2a

	case compute::CalendarUnit::YEAR: {
	year_month_day ymd(
	floor<days>(localizer_.template ConvertTimePoint<Duration>(arg)));
	year y{(static_cast<int32_t>(ymd.year()) / options.multiple) * options.multiple};
	t = localizer_.template ConvertLocalToSys<Duration>(
	local_days(y / jan / 1).time_since_epoch(), st);
	break;

If I remember correctly we have lubridate inspired calendar_based_rounding:

arrow/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Lines 752 to 754 in 067fd2a

	// Round to a multiple of units since the last greater unit.
	// For example: round to multiple of days since the beginning of the month or
	// to hours since the beginning of the day.

And not calendar_based_rounding:

arrow/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Lines 798 to 799 in 067fd2a

	// Round to a multiple of units * options.multiple since epoch start
	// (1970-01-01 00:00:00).

Rounding to years in not calendar_based_rounding should then use 1970-01-01 as origin indeed although one could argue it's bit of an exception. I'll open a PR.

[AlenkaF]

I'm not entirely sure what the expected behavior should be in this case, but it might make sense to align it with how similar operations work in pandas—for example, pandas.Series.dt.floor.

Note that the week unit behaves similarly (there may be other units as well; a parameterized test could help reveal this):

>>> s = pa.scalar(datetime(1970, 1, 1), type=pa.timestamp('ns'))
>>> print(pc.floor_temporal(s, 1, 'week'))
1969-12-29 00:00:00

[MarcoGorelli]

thanks for looking into this

pandas doesn't support flooring by year

[AlenkaF]

pandas doesn't support flooring by year

Ah, so this is the meaning of this part (from the docs):

Must be a fixed frequency like ‘S’ (second) not ‘ME’ (month end).

So no "end" or "start" frequencies are supported, I presume.

[MarcoGorelli]

Yup, exactly. The often-recommended workaround is to via 'Period', but I don't trust Period dtype in pandas one bit, so in Narwhals we go down to the NumPy / PyArrow level https://github.com/narwhals-dev/narwhals/blob/840d67ea131f43c6820aaaadb843b9aa42513cd2/narwhals/_pandas_like/series_dt.py#L210-L233

[AlenkaF]

Ah, this makes things clearer—thanks!
Happy to help with some guidance, @rok , if you think it's something I can do 😊

[rok]

Thinking about this some more - 1970-01-01 is a very arbitrary origin point for most applications. There is nothing meaningful about it except for the fact it's an internal implementation detail. (Which could be said of year 0 too, but year 0 should feel fore intuitive generally). But pyarrow docs does indeed state "By default, the origin is 1970-01-01T00:00:00" for "not calendar_based_origin" so it would be best to change this.

As an aside - it would be very valuable to have intra-library tests for ceil/round/floor behavior that would compare results on the "full" time continuum (from say 2000 BC to 3000 AD, uniformly sampled, with a known seed, on a resolution lower than the unit being rounded too). As this is hard currently (e.g. pandas not flooring to years, some libraries not rounding in local time) next best thing would be to have a comprehensive test dataset.

@AlenkaF I'd be happy to help you want to do this.

Note that the week unit behaves similarly (there may be other units as well; a parameterized test could help reveal this):

>>> s = pa.scalar(datetime(1970, 1, 1), type=pa.timestamp('ns'))
>>> print(pc.floor_temporal(s, 1, 'week'))
1969-12-29 00:00:00

Well, 1969-12-29 was a Monday according to google, so this is correct (if painful to think about).

@MarcoGorelli do you intend to support rounding to multiple time units in local time in narwhals?

[MarcoGorelli]

yup our objective is to always round in local time

[rok]

@MarcoGorelli great to hear! I plan to finish #12528 at some point, would you mind to review when I get back to it?

[MarcoGorelli]

if i can understand it, sure 😄