Continued ujson cleanups #26222
Continued ujson cleanups #26222
Conversation
| | ||
| #define NPY_JSON_BUFSIZE 32768 | ||
| | ||
| static PyTypeObject *type_decimal; |
There was a problem hiding this comment.
This was previously a static PyObject, but the CPython guide says you should never declare static PyObjects. Swapped over to PyTypeObject to match semantics of others in the extension:
https://docs.python.org/3/c-api/intro.html#objects-types-and-reference-counts
| obj = (PyArrayObject *)_obj; | ||
| } | ||
| | ||
| if (PyArray_SIZE(obj) < 0) { |
There was a problem hiding this comment.
This branch of the condition was unreachable
There was a problem hiding this comment.
@shoyer is there some special case this might have been intended for?
There was a problem hiding this comment.
I'm not familiar with the context here, but I agree that this was unreachable.
Maybe it was intended to cover size 0 arrays, may not be representable in JSON? e.g., an shape with shape (0, 1)
| } | ||
| | ||
| return; | ||
| } else if (PyLong_Check(obj)) { |
There was a problem hiding this comment.
Also unreachable; this used to be PyInt_Check so could yield something in Python2, but with switch to Python3 was duplicative of the PyLong_Check that immediately preceded it
Codecov Report
@@ Coverage Diff @@ ## master #26222 +/- ## ========================================== - Coverage 91.97% 91.97% -0.01% ========================================== Files 175 175 Lines 52379 52379 ========================================== - Hits 48178 48174 -4 - Misses 4201 4205 +4
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@ ## master #26222 +/- ## ========================================== - Coverage 91.97% 91.97% -0.01% ========================================== Files 175 175 Lines 52379 52379 ========================================== - Hits 48178 48174 -4 - Misses 4201 4205 +4
Continue to review full report at Codecov.
|
| pc->PyTypeToJSON = PyUnicodeToUTF8; | ||
| tc->type = JT_UTF8; | ||
| return; | ||
| } else if (PyObject_IsInstance(obj, type_decimal)) { |
There was a problem hiding this comment.
are there any Decimal subclasses to worry about?
There was a problem hiding this comment.
PyObject_TypeCheck returns true for subtypes as well, so I don't think that matters:
https://docs.python.org/3.7/c-api/object.html#c.PyObject_TypeCheck
jreback left a comment
jreback left a comment There was a problem hiding this comment.
do we have sufficient tests with serializing Decimal?
| All of the decimal tests are located here: pandas/pandas/tests/io/json/test_ujson.py Line 62 in 51980fe They don't appear in anything user-facing but are covered in the encode/decode round tripping that this would effect. If Decimal subclasses are a concern can also parametrize that test for Decimal and an arbitrary subclass. Let me know. |
| thanks @WillAyd |
4 participants
ref some of the comments by @jbrockmendel in #26212