BUG: Index with null value not serialized correctly to json #50400
Conversation
| } else { | ||
| tc->type = JT_NULL; | ||
| } | ||
| Py_DECREF(is_null_obj); |
There was a problem hiding this comment.
In a case where PyObject_CallMethod returns NULL this would try to Py_DECREF NULL
| if (PyFloat_Check(item)) { | ||
| double fval = PyFloat_AS_DOUBLE(item); | ||
| is_null = npy_isnan(fval); | ||
| } else if (item == Py_None || object_is_na_type(item)) { |
There was a problem hiding this comment.
Do we have a similar expression already in missing.pyx? I wonder if there's a way for us to be consistent about our missing object evaluation
There was a problem hiding this comment.
checknull is probably the equivalent. Without having a C API, I don't really think we can unify anything sadly.
There was a problem hiding this comment.
Gotcha. Not a blocker here, but it might be nice to create a separate missing.c file with the proper implementation and then can wrap that from missing.pyx
There was a problem hiding this comment.
On a second read I think we should at least create a separate function for int is_null(PyObject *obj) here. Can just be static in the current translation unit - makes the code more readable than the nested if statements
| "is_nan", | ||
| NULL); | ||
| is_null = (is_null_obj == Py_True); | ||
| Py_DECREF(is_null_obj); |
There was a problem hiding this comment.
This will likely segfault if PyObject_CallMethod returns NULL
lithomas1
added
Bug
IO JSON
| } else { | ||
| tc->type = JT_NULL; | ||
| } | ||
| Py_XDECREF(is_null_obj); |
There was a problem hiding this comment.
Instead of using Py_XDECREF we should explicitly check the return value of PyObject_CallMethod and return an error if it is NULL
| } else { | ||
| tc->type = JT_NULL; | ||
| } | ||
| if (!is_null_obj) { |
There was a problem hiding this comment.
I think you should move this up to 1542 before the condition of equating to Py_False. This makes for a more consistent pattern where we always check for NULL after a Py* call
| if (PyFloat_Check(item)) { | ||
| double fval = PyFloat_AS_DOUBLE(item); | ||
| is_null = npy_isnan(fval); | ||
| } else if (item == Py_None || object_is_na_type(item)) { |
There was a problem hiding this comment.
On a second read I think we should at least create a separate function for int is_null(PyObject *obj) here. Can just be static in the current translation unit - makes the code more readable than the nested if statements
| @@ -276,6 +276,27 @@ static int is_simple_frame(PyObject *obj) { | |||
| Py_DECREF(mgr); | |||
| return ret; | |||
| } | |||
| /* TODO: Consider unifying with checknull and co. | |||
There was a problem hiding this comment.
Can you add a quick note in the docstring that this returns -1 on error?
| @@ -276,6 +276,27 @@ static int is_simple_frame(PyObject *obj) { | |||
| Py_DECREF(mgr); | |||
| return ret; | |||
| } | |||
| /* TODO: Consider unifying with checknull and co. | |||
| in missing.pyx */ | |||
| static int is_null_obj(PyObject* obj) { | |||
There was a problem hiding this comment.
Nit but I think this should be called is_json_null since its semantics may vary from what we have elsewhere in the codebase
| if (is_null == -1) { | ||
| // Something errored | ||
| // Return to let the error surface | ||
| return 0; |
There was a problem hiding this comment.
Surprised we return 0 here but I see that you are just matching the pattern of the rest of the function. It should really be returning NULL in case of an error - looks to not be handled properly. But of course that is separate from this PR
| @@ -1539,15 +1552,15 @@ void Object_beginTypeContext(JSOBJ _obj, JSONTypeContext *tc) { | |||
| PyObject *is_null_obj = PyObject_CallMethod(obj, | |||
There was a problem hiding this comment.
Can this not be replaced with the function you are introducing? Seems like it should work to keep logic consistent?
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.Restores 0.25.3 behavior, which honestly feels a little weird to me, since it doesn't roundtrip. I'm not too familiar with JSON format, though (I guess the problem is that null can't be a key?).
Also serialize decimal nans correctly.