BUG: Fix pd.json_normalize to not skip the first element of a generator input #38698
Conversation
|
Consuming the generator seemed the easiest way to me. Since we also went for this approach in other location of the codebase I used it here as well. Please LMK if we want to solve this is another way, i.e. not consuming the whole generator at once. |
pandas/io/json/_normalize.py
Outdated
| @@ -267,6 +267,11 @@ def _pull_records(js: Dict[str, Any], spec: Union[List, str]) -> List: | |||
| data = [data] | |||
|
|
|||
| if record_path is None: | |||
| if np.ndim(data) == 0: | |||
There was a problem hiding this comment.
What about inserting a call to next(data) to skip past the first element?
It's always good to be as lazy as possible but tbf I don't know how much that matters here
There was a problem hiding this comment.
@arw2019 wdym with skipping past the first line with "next(data)"? Cause we do not want to skip the first element.
Below I've written down my understanding and solution to the problem. But lmk if you see this differently!
The problem here is that in the old situation the first line of an input generator was consumed and not part of the output dataframe. This was caused by this line: https://github.com/pandas-dev/pandas/blob/master/pandas/io/json/_normalize.py#L270.
By consuming the whole generator and putting it in a list we don't have that issue anymore
There was a problem hiding this comment.
Okay right your solution looks good
(There's actually already a TODO there about expanding the generator with nested records, as well)
There was a problem hiding this comment.
Ah great. Wrt to the TODO, which one do you mean? I see this one: TODO: handle record value which are lists, at least error reasonably, but IMO that would justify its own PR since it is related to a different issue
There was a problem hiding this comment.
Agreed, that TODO is beyond the scope of this PR
There was a problem hiding this comment.
you can just put a list(data) on L264 (you could even limit this to a Iterable type if you want)
There was a problem hiding this comment.
this entire function needs to be split up into module level functions and cleaned up. I believe we have several open issues about this (but orthogonal to this PR).
There was a problem hiding this comment.
Put list(data) on L264, but limited it to type Iterator and not Iterable since we use different conversion logic for dicts
Wrt to the splitting of the modules. I will add it to my list, but first I want to address some other PRs for dtypes.
|
Failures in CI caused by #38703 |
pandas/io/json/_normalize.py
Outdated
| @@ -267,6 +267,11 @@ def _pull_records(js: Dict[str, Any], spec: Union[List, str]) -> List: | |||
| data = [data] | |||
|
|
|||
| if record_path is None: | |||
| if np.ndim(data) == 0: | |||
There was a problem hiding this comment.
you can just put a list(data) on L264 (you could even limit this to a Iterable type if you want)
pandas/io/json/_normalize.py
Outdated
| @@ -267,6 +267,11 @@ def _pull_records(js: Dict[str, Any], spec: Union[List, str]) -> List: | |||
| data = [data] | |||
|
|
|||
| if record_path is None: | |||
| if np.ndim(data) == 0: | |||
There was a problem hiding this comment.
this entire function needs to be split up into module level functions and cleaned up. I believe we have several open issues about this (but orthogonal to this PR).
pandas/io/json/_normalize.py
Outdated
| @@ -262,6 +262,11 @@ def _pull_records(js: Dict[str, Any], spec: Union[List, str]) -> List: | |||
| if isinstance(data, list) and not data: | |||
| return DataFrame() | |||
|
|
|||
| if isinstance(data, abc.Iterator): | |||
There was a problem hiding this comment.
can you make these if/elif (all 3 conditions)
pandas/io/json/_normalize.py
Outdated
| elif isinstance(data, abc.Iterator): | ||
| # GH35923 Fix pd.json_normalize to not skip the first element of a | ||
| # generator input | ||
| data = list(data) |
There was a problem hiding this comment.
This could have some big performance implications when dealing with large generators - is it not alternately possible to just store the first element for inspection and reuse as necessary while maintaining the state of the generator?
There was a problem hiding this comment.
we barely support generators (its not even documented), so -1 if this adds any complexity.
|
Fair point. I might be misremembering but I feel like we do something similar for other IO methods, so a common way of doing that would be nice. But agreed for scope of this PR converting to list is fine
… On Dec 29, 2020, at 10:39 AM, Jeff Reback ***@***.***> wrote:
@jreback commented on this pull request.
In pandas/io/json/_normalize.py <#38698 (comment)>:
> @@ -261,9 +261,12 @@ def _pull_records(js: Dict[str, Any], spec: Union[List, str]) -> List:
if isinstance(data, list) and not data:
return DataFrame()
-
- # A bit of a hackjob
- if isinstance(data, dict):
+ elif isinstance(data, abc.Iterator):
+ # GH35923 Fix pd.json_normalize to not skip the first element of a
+ # generator input
+ data = list(data)
we barely support generators (its not even documented), so -1 if this adds any complexity.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#38698 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU4UOCSISA6VXUMZEHTMLSXIO4RANCNFSM4VJFUZVA>.
|
|
thanks @avinashpancham |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff