BUG: Fix json_normalize throwing TypeError when record_path has a sequence of dicts #22706

[vuminhle]

• closes json_normalize raises TypeError exception #22706
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @vuminhle! Thanks for updating the PR.

• There are no PEP8 issues in the file pandas/io/json/normalize.py !

• There are no PEP8 issues in the file pandas/tests/io/json/test_normalize.py !

Comment last updated on September 26, 2018 at 04:20 Hours UTC

[codecov]

Codecov Report

Merging #22804 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22804      +/-   ##
==========================================
+ Coverage   92.21%   92.21%   +<.01%     
==========================================
  Files         162      162              
  Lines       51763    51770       +7     
==========================================
+ Hits        47733    47741       +8     
+ Misses       4030     4029       -1

Flag	Coverage Δ
#multiple	90.62% <100%> (ø)	⬆️
#single	42.99% <0%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/io/json/normalize.py	96.93% <100%> (+0.06%)	⬆️
pandas/core/generic.py	96.65% <0%> (ø)	⬆️
pandas/core/groupby/generic.py	87.12% <0%> (+0.04%)	⬆️
pandas/core/groupby/groupby.py	96.67% <0%> (+0.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91802fb...db24124. Read the comment docs.

[vuminhle]

Any other feedback @WillAyd @jreback ?

[WillAyd]

Code changes lgtm. Some minor nits around the docs

[vuminhle]

I think this PR also fixes #21605

[WillAyd]

Can you merge master?

[vuminhle]

Can you merge master?

Done.

[WillAyd]

Is there a reason for both _extract and _recursive_extract? Pretty confusing IMO would be better served as one consolidated function I think

[vuminhle]

Is there a reason for both _extract and _recursive_extract? Pretty confusing IMO would be better served as one consolidated function I think

The new _extract performs the actual record extraction. I can consolidate the two but we need to convert data to list in case it is not. Then we can iterate on data as in the original code. It's the same hack as line 198. _recursive_extract will look roughly like this:

if len(path) > 1:
    ...
else:
    if not isinstance(data, list):
        data = [data]
    for obj in data:
        # perform actual extraction

Since we only have 1 function now, I think we can revert the change of returning tuples. IMO it looks ugly. What do you think?

[WillAyd]

I can consolidate the two but we need to convert data to list in case it is not

I'm pretty sure this is done quite a few other places so not out of the ordinary, though feel free to double check.

Since we only have 1 function now, I think we can revert the change of returning tuples. IMO it looks ugly. What do you think?

Not sure I fully understand what you have in mind but yea I don't think as is this is very readable. Any refactor / simplification here which does away with similar but separate functions and does not require mutation of an argument would be a great improvement

[vuminhle]

I can consolidate the two but we need to convert data to list in case it is not

I'm pretty sure this is done quite a few other places so not out of the ordinary, though feel free to double check.

Since we only have 1 function now, I think we can revert the change of returning tuples. IMO it looks ugly. What do you think?

Not sure I fully understand what you have in mind but yea I don't think as is this is very readable. Any refactor / simplification here which does away with similar but separate functions and does not require mutation of an argument would be a great improvement

Can you take another look at the changes? Since _recursive_extract assumes that data is always a list, I added 2 lines to convert data to list if it is not.

[jreback]

this also fixed #21605? can you add a test and add to the whatsnew

[vuminhle]

this also fixed #21605? can you add a test and add to the whatsnew

Looks like #21605 is a duplicate (of #22804 ). We can just close #21605.

[jreback]

hmm, though that is a list of dicts?

[vuminhle]

hmm, though that is a list of dicts?

list of dicts should work already.

[jreback]

ok this lgtm then. @WillAyd over to you.