BUG: to_json not serializing non-nanosecond numpy dt64 correctly #53757

lithomas1 · 2023-06-21T01:08:32Z

closes BUG: to_json returns incorrect timestamps if DatetimeIndex precision is not ns #53686 (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.

lithomas1 · 2023-06-21T01:10:18Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

+            castfunc(dataptr, &i8date, 1, NULL, NULL);
+            // copied from
+            // https://github.com/numpy/numpy/blob/c8fe278a754a271af57eaf6c7ffb2382e5a954f9/numpy/core/src/multiarray/datetime.c#L692-L701
+            dateUnit = ((PyArray_DatetimeDTypeMetaData *)dtype->c_metadata)->meta.base;


This part is probably the most sketchy part of this PR.

We currently do this the exact same way numpy does it, but not sure if this is part of the public numpy C API.

FWIW, PyArray_DatetimeDTypeMetaData appears in ndarraytypes.h.

It would be better if we stuck to a method exposed in the datetime header. Does get_datetime_metadata_from_dtype not help here?

It would be nice to use that, not sure if that's public either (it's defined in multiarray/_datetime.h).
Like PyArray_DatetimeDTypeMetaData, it also seems to be undocumented. Maybe one of the numpy maintainers(@seberg?) can clarify more?

EDIT: NVM, didn't see we had this internally. Sorry for the ping.

WillAyd

Very nice

pandas/_libs/include/pandas/datetime/date_conversions.h

WillAyd · 2023-06-21T03:53:58Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

+            castfunc(dataptr, &i8date, 1, NULL, NULL);
+            // copied from
+            // https://github.com/numpy/numpy/blob/c8fe278a754a271af57eaf6c7ffb2382e5a954f9/numpy/core/src/multiarray/datetime.c#L692-L701
+            dateUnit = ((PyArray_DatetimeDTypeMetaData *)dtype->c_metadata)->meta.base;


It would be better if we stuck to a method exposed in the datetime header. Does get_datetime_metadata_from_dtype not help here?

…-non-nano

jbrockmendel · 2023-06-21T20:40:33Z

@lithomas1 is there a viable way to do this using values_for_json, i.e. keep this complexity out of the C code?

lithomas1 · 2023-06-21T21:42:55Z

@lithomas1 is there a viable way to do this using values_for_json, i.e. keep this complexity out of the C code?

Long term, we could move to using to_native_types like CSV does, and just directly feed strings to the JSON serializer (for datetime/EA types).

I don't think this PR adds too much complexity (the core changes are just ~3 lines), so maybe we can merge this for now, and then try to pull the logic for anything other than native types (e.g. ints, floats) out of the JSON C code?

WillAyd · 2023-06-21T23:27:52Z

The challenge with values_for_json is also that to_json has keyword arguments that control the JSON output, e.g. date_format and date_unit. Those are both handled in the serializer currently; values_for_json would have to handle those arguments generically across EAs

jbrockmendel · 2023-06-24T19:58:12Z

values_for_json would have to handle those arguments generically across EAs

that sounds like a better long-term solution than doing more here (assuming perf is OK). I'm happy to defer to you two on this PR in the interim.

…-non-nano

WillAyd · 2023-07-19T02:51:53Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

@@ -1293,7 +1301,8 @@ char **NpyArr_encodeLabels(PyArrayObject *labels, PyObjectEncoder *enc,
        }

        int is_datetimelike = 0;
-        npy_int64 nanosecVal;
+        npy_int64 i8date;
+        NPY_DATETIMEUNIT dateUnit = NPY_FR_ns;


Is the idea here that this should default to NS for things that are stored within object arrays?

It's for the other stuff like timedeltas and dates. We use nanosecond reso there.

I didn't try to fix those cases as well.

What do you think it would take to handle those consistently? I don't know how common this would happen but I guess its strange to only do this for numpy-typed arrays and not object arrays that may contain datetimes

knee-jerk i think that for object-dtype we should just call str(x) on everything and call it a day

The object stuff definitely adds a lot of complexity that may not be worth it, but that would be a breaking change to just pass as a str. Particularly for nested containers within an object array that would not work

I think it might work for object arrays, already.

I haven't checked yet, I'm not on the right branch. Will update later in the day.

Well, it turns out that this didn't work already. Sorry for the confusion.

I've fixed this in the latest commit.

…-non-nano

lithomas1 · 2023-07-23T15:34:27Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

-        PyErr_SetString(PyExc_TypeError, "Expected date object");
+    if (!PyDate_Check(obj) && !PyDateTime_Check(obj)) {
+        PyErr_SetString(PyExc_TypeError, "Expected date or datetime object");
+        ((JSONObjectEncoder *)tc->encoder)->errorMsg = "";


Off-topic, but we should be setting the errorMsg whenever we return NULL in a callback.

Otherwise, there is a possibility of a segfault, since ujson will still think it's in a valid state and try to continue encoding, instead of instantly returning (allowing the Python exception to propagate up).

lithomas1 · 2023-07-23T15:57:51Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

        if (((PyDatetimeScalarObject *)obj)->obval == get_nat()) {
            tc->type = JT_NULL;
            return;
        }
+        PyArray_Descr *dtype = PyArray_DescrFromScalar(obj);
+        if (dtype->type_num == NPY_OBJECT) {


Wondering if this can be removed. This shouldn't ever fail, but if it does, we'll end up with a nasty segfault.

https://numpy.org/doc/stable/reference/c-api/array.html#c.PyArray_DescrFromScalar

Reading through that makes me think we are supposed to check we are dealing with an array scalar before even calling this function,

As far as the resulting comparison to NPY_OBJECT, I think would be better to explicitly use the PyTypeNum_ISDATETIME macro and error when that is not true; that would be a better and more explicit safeguard against troublesome refactors

Yeah, this whole block is in a PyArray_IsScalar if block.

WillAyd

lgtm. minor comments but OK with merging whenever you feel complete

WillAyd · 2023-07-24T03:37:41Z

pandas/_libs/src/vendored/ujson/python/objToJSON.c

        if (((PyDatetimeScalarObject *)obj)->obval == get_nat()) {
            tc->type = JT_NULL;
            return;
        }
+        PyArray_Descr *dtype = PyArray_DescrFromScalar(obj);
+        if (dtype->type_num == NPY_OBJECT) {
+            PyErr_Format(PyExc_ValueError, "Could not get resolution of datetime");


Any reason we aren't returning here? Feels a bit off to let execution continue after setting error

My bad, updated.

lithomas1 · 2023-07-25T22:07:27Z

gonna put this in then.

Thanks for the reviews all.

…das-dev#53757) * BUG: to_json not serializing non-nanosecond numpy dt64 correctly * fix tests * change extraction mech * fix object array case * pre-commit * address comments

BUG: to_json not serializing non-nanosecond numpy dt64 correctly

e648188

lithomas1 added the IO JSON read_json, to_json, json_normalize label Jun 21, 2023

lithomas1 commented Jun 21, 2023

View reviewed changes

lithomas1 added 2 commits June 20, 2023 20:31

fix tests

4136ce1

Merge branch 'main' into json-non-nano

c0e745d

lithomas1 marked this pull request as ready for review June 21, 2023 03:33

lithomas1 requested a review from WillAyd as a code owner June 21, 2023 03:33

WillAyd reviewed Jun 21, 2023

View reviewed changes

lithomas1 added 2 commits June 21, 2023 09:59

change extraction mech

4759340

Merge branch 'json-non-nano' of github.com:lithomas1/pandas into json…

13ddc60

…-non-nano

lithomas1 added 3 commits July 6, 2023 14:05

Merge branch 'main' of https://github.com/pandas-dev/pandas into json…

b45657b

…-non-nano

Merge branch 'main' into json-non-nano

190f43a

Merge branch 'json-non-nano' of github.com:lithomas1/pandas into json…

cb08985

…-non-nano

WillAyd reviewed Jul 19, 2023

View reviewed changes

WillAyd mentioned this pull request Jul 19, 2023

Add support for date_unit="D" in read_json and to_json #51000

Closed

5 tasks

lithomas1 added 3 commits July 19, 2023 20:53

Merge branch 'main' into json-non-nano

71fadf5

Merge branch 'json-non-nano' of github.com:lithomas1/pandas into json…

d95e7cf

…-non-nano

fix object array case

f861b6a

lithomas1 commented Jul 23, 2023

View reviewed changes

pre-commit

c3299de

WillAyd approved these changes Jul 24, 2023

View reviewed changes

address comments

c9da070

lithomas1 merged commit ee5cf2c into pandas-dev:main Jul 25, 2023

lithomas1 deleted the json-non-nano branch July 25, 2023 22:07

mroeschke added this to the 2.1 milestone Jul 25, 2023

lithomas1 mentioned this pull request Dec 3, 2023

BUG/API: round-tripping non-nano datetime64s with to_json/read_json #55827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: to_json not serializing non-nanosecond numpy dt64 correctly #53757

BUG: to_json not serializing non-nanosecond numpy dt64 correctly #53757

lithomas1 commented Jun 21, 2023

lithomas1 Jun 21, 2023

WillAyd Jun 21, 2023

lithomas1 Jun 21, 2023 •

edited

Loading

WillAyd left a comment

WillAyd Jun 21, 2023

jbrockmendel commented Jun 21, 2023

lithomas1 commented Jun 21, 2023

WillAyd commented Jun 21, 2023 •

edited

Loading

jbrockmendel commented Jun 24, 2023

WillAyd Jul 19, 2023

lithomas1 Jul 19, 2023

WillAyd Jul 19, 2023

jbrockmendel Jul 19, 2023

WillAyd Jul 19, 2023

lithomas1 Jul 19, 2023

lithomas1 Jul 23, 2023

lithomas1 Jul 23, 2023

lithomas1 Jul 23, 2023

WillAyd Jul 24, 2023

lithomas1 Jul 24, 2023

WillAyd left a comment

WillAyd Jul 24, 2023

lithomas1 Jul 24, 2023

lithomas1 commented Jul 25, 2023

BUG: to_json not serializing non-nanosecond numpy dt64 correctly #53757

BUG: to_json not serializing non-nanosecond numpy dt64 correctly #53757

Conversation

lithomas1 commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lithomas1 Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jun 21, 2023

lithomas1 commented Jun 21, 2023

WillAyd commented Jun 21, 2023 • edited Loading

jbrockmendel commented Jun 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lithomas1 commented Jul 25, 2023

lithomas1 Jun 21, 2023 •

edited

Loading

WillAyd commented Jun 21, 2023 •

edited

Loading