Skip to content

Address Untested to_json Extension Module Code #28273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
WillAyd opened this issue Sep 3, 2019 · 4 comments
Closed

Address Untested to_json Extension Module Code #28273

WillAyd opened this issue Sep 3, 2019 · 4 comments
Labels
good first issue IO JSON read_json, to_json, json_normalize Needs Tests Unit test(s) needed to prevent regressions

Comments

@WillAyd
Copy link
Member

WillAyd commented Sep 3, 2019

There's an old TODO note about untested code in the extension module hanging around in objToJSON.c:

// TODO(anyone): Does not appear to be reached in tests.

Interestingly enough, this is actually tested by the np.datetime64 parameter in test_encode_as_null as part of the test_ujson.py module:

def test_encode_as_null(self, decoded_input):

The intent here is somewhat blurry. I think the test_ujson.py module was created when it was up for discussion if we wanted the JSON serializers to be publicly exposed at the top level (see #9147) which I don't believe is still something we want to do. At that time it would make sense to explicitly support the np.datetime64 type.

The remaining question then is whether np.datetime64 should be represented within a pandas container. It seems that this can be held in a Series with an object dtype but not a DataFrame, as shown below:

>>> ser = pd.Series([np.datetime64("2000-01-01")], dtype=object)
>>> type(ser.iloc[0])
numpy.datetime64
>>> type(ser.to_frame().iloc[0, 0])
pandas._libs.tslibs.timestamps.Timestamp

If we don't want np.datetime64 objects to be contained then we can delete this code altogether. If we do we should add test coverage for it, but then also address the inconsistency across container types above

@WillAyd WillAyd added the Needs Discussion Requires discussion from core team before further action label Sep 3, 2019
@jbrockmendel
Copy link
Member

Interestingly enough, this is actually tested by the np.datetime64 parameter in test_encode_as_null as part of the test_ujson.py module:

I'm pretty sure I added that TODO note. IIRC I commented out the relevant code and found that it didn't break any tests. So its conceivable that it is reached in a test, but it is a smoke-test?

if the note is just inaccurate, feel free to remote it.

@WillAyd
Copy link
Member Author

WillAyd commented Sep 4, 2019

Maybe commenting has a different effect but it definitely causes a failure if outright removed in test_encode_as_null from test_ujson.py when dealing with an np.datetime64 object

The only way to really add a test for this would be to create a Series with a np.datetime64 item and object dtype; any other path I think implicitly converts the np.datetime64 to a pandas object.

So could add that Series test though not sure what the overall intent of date time handling is

@jbrockmendel jbrockmendel added the IO JSON read_json, to_json, json_normalize label Feb 25, 2020
@mroeschke mroeschke added the Testing pandas testing functions or related to the test suite label May 8, 2020
@mroeschke mroeschke added Needs Tests Unit test(s) needed to prevent regressions good first issue and removed Testing pandas testing functions or related to the test suite Needs Discussion Requires discussion from core team before further action labels Jul 1, 2022
@Karthik-PM
Copy link

can i work on this issue

@mroeschke
Copy link
Member

Looks like this code doesn't exist anymore in objToJSON, so guess we can close this one out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue IO JSON read_json, to_json, json_normalize Needs Tests Unit test(s) needed to prevent regressions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants