API: Timestamp and Timedelta .value changing in 2.0

[MarcoGorelli]

• closes API: Timestamp and Timedelta .value changing in 2.0 #49076 (Replace xxxx with the GitHub issue number)
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[MarcoGorelli]

failed test:

FAILED pandas/tests/io/json/test_pandas.py::TestPandasContainer::test_date_index_and_values[date-True-epoch] - assert '{"1577836800...null,"a":"a"}' == '{"1577836800...null,"a":"a"}' - {"1577836800000":1577836800000,"null":null,"a":"a"} ? ^^^^ + {"1577836800000":1577836800000,"-6858695778871":null,"a":"a"} ? 

can't get my head round it - cc @jbrockmendel @WillAyd in case you have any ideas, else I'll keep at it

renaming .value affects so much of the codebase..

[jbrockmendel]

can't get my head round it - cc @jbrockmendel @WillAyd in case you have any ideas, else I'll keep at it

best guess is in objToJSON.c there are a couple of get_long_attr(item, "value") calls that might not be caught by grepping for .value

[WillAyd]

I think the JSON module returns garbage values because of #49756

If you look, there are some branches in the code that do something like:

 if (PyObject_HasAttrString(item, "value")) { // see test_date_index_and_values for case with non-nano nanosecVal = get_long_attr(item, "value"); } else { if (PyDelta_Check(item)) { nanosecVal = total_seconds(item) * 1000000000LL; // nanoseconds per second } else { // datetime.* objects don't follow above rules nanosecVal = PyDateTimeToEpoch(item, NPY_FR_ns); } }

For the timedelta, this goes into total_seconds which is implemented as such:

static npy_float64 total_seconds(PyObject *td) { npy_float64 double_val; PyObject *value = PyObject_CallMethod(td, "total_seconds", NULL); double_val = PyFloat_AS_DOUBLE(value); Py_DECREF(value); return double_val; }

I'm guessing that the object doesn't actually have total_seconds defined, and you end up getting pretty crazy results.

cc @lithomas1 who may be interested

[WillAyd]

As the master of code checks @MarcoGorelli if you had some kind of idea on how to set up CI so that all result = PyObject_... C functions are immediately followed by an if (result == NULL) { // handle error } that could help us clean up our extensions. Otherwise its probably worth doing a manual review of things as a pre-cursor to this PR

[MarcoGorelli]

Thanks, get_long_attr(item, "value") was indeed the part I needed to change!

As the master of code checks @MarcoGorelli if you had some kind of idea on how to set up CI so that all result = PyObject_... C functions are immediately followed by an if (result == NULL) { // handle error } that could help us clean up our extensions

😄 I'll take a look

[MarcoGorelli]

gonna block myself here, want to make sure this is really what's desired, as then the public method .value would become unavailable for old dates: #49076 (comment)

[MarcoGorelli]

gonna block myself here, want to make sure this is really what's desired, as then the public method .value would become unavailable for old dates: #49076 (comment)

have amended the error message to suggest using asm8 if nanoseconds aren't what they're after

[jbrockmendel]

to be Technically Correct do we need to make value a property here?

[jbrockmendel]

i think this one is expecting the nanos value? (and probably needs to be updated to allow non-nano?)

[jbrockmendel]

looks like 1) this scalar path might not be necessary? and 2) there may be baked-in assumptions about everything being nano

[MarcoGorelli]

good call - shall we make logic changes in a separate PR? I've opened #51196 about this nanosecond assumption here

[jbrockmendel]

separate PR seems fine

[MarcoGorelli]

sure, thanks - any objections here? shall we move forward before merge conflicts arise?

[mroeschke]

Looks fairly good. Could this use a whatsnew note about .values raising a OverflowError for non nano unit?

[MarcoGorelli]

thanks - @jbrockmendel can confirm but I don't think this (non-nano timestamp) would've been available anyway in 1.5.x

[jbrockmendel]

I don't think this (non-nano timestamp) would've been available anyway in 1.5.x

correct

[jbrockmendel]

LGTM

[jbrockmendel]

thanks @MarcoGorelli