Skip to content

Improve ujson test coverage or clean up dead code #26212

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 Apr 25, 2019 · 7 comments
Closed

Improve ujson test coverage or clean up dead code #26212

WillAyd opened this issue Apr 25, 2019 · 7 comments
Labels
IO JSON read_json, to_json, json_normalize

Comments

@WillAyd
Copy link
Member

WillAyd commented Apr 25, 2019

I've been spending some time cleaning up the integration with ujson in our source code. After enabling some debugging directives and running the test suite I noticed that there were quite a few places in objToJSON.c which were not being hit. This can point to either dead code or a lack of test coverage, so these can be whittled down.

In my own repo I have a branch of this where each of the items are commented with // not hit in tests which you can see here:

https://github.com/WillAyd/pandas/blob/dead-json/pandas/_libs/src/ujson/python/objToJSON.c

Planning to go through these individually and see what we can do. Extra set of eyes would certainly be welcome

@WillAyd
Copy link
Member Author

WillAyd commented Apr 25, 2019

@jbrockmendel you might be interested

@WillAyd WillAyd added the IO JSON read_json, to_json, json_normalize label Apr 25, 2019
@TomAugspurger
Copy link
Contributor

I’m hopeful that we can eventually remove all of this and use arrow and it’s JSON support. Given that, it may not be worthwhile spending too much time here.

@jbrockmendel
Copy link
Member

@TomAugspurger any idea what "eventually" means in this context? i.e. are we talking months or years?

@TomAugspurger
Copy link
Contributor

https://issues.apache.org/jira/browse/ARROW-694 was the initial blocker, and that just landed in arrow 0.13.0. So, with a good amount of effort, months seems reasonable.

@jbrockmendel
Copy link
Member

jbrockmendel commented Apr 25, 2019

Extra set of eyes would certainly be welcome

Yah, there's definitely some remove-able stuff in there.

  • L65-67 is a check for py<2.5
  • L163-167 is a PY3 check
  • is the NPY_API_VERSION < 0.7 check on L729 removable?
  • i have no idea what the semantics are for L737-753
  • L648 when would obj.size be negative?
  • is the PyLong_Check on 1785 redundant with the check on L1771?

@WillAyd
Copy link
Member Author

WillAyd commented Apr 25, 2019

Yep thanks @jbrockmendel . FYI some of the compat stuff I already ripped out in #26211

@WillAyd
Copy link
Member Author

WillAyd commented Sep 5, 2019

This has been / is being tackled elsewhere

@WillAyd WillAyd closed this as completed Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO JSON read_json, to_json, json_normalize
Projects
None yet
Development

No branches or pull requests

3 participants