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
| @@ -628,44 +627,39 @@ void NpyArr_iterBegin(JSOBJ _obj, JSONTypeContext *tc) { | |||
| 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)
| @@ -1750,17 +1744,6 @@ void Object_beginTypeContext(JSOBJ _obj, JSONTypeContext *tc) { | |||
| goto INVALID; | |||
| } | |||
|
|
|||
| 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.
|
| @@ -1782,7 +1765,7 @@ void Object_beginTypeContext(JSOBJ _obj, JSONTypeContext *tc) { | |||
| 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
|
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 |
ref some of the comments by @jbrockmendel in #26212