Add chunksize param to read_json when lines=True
State: closed

Previous behavior: reading the whole file to memory and then split into lines.
New behavior: if `lines=True` and `chunksize` is passed: read in `chunksize` lines at a time, and concat.

This only covers some kinds of input to `read_json`.  When `chunksize` is passed, `read_json` becomes slower but more memory-efficient.

Closes #17048.

Tests and style-check pass, no new tests added.

Labels:
name: Performance
description: Memory or execution speed performance
name: IO JSON
description: read_json, to_json, json_normalize

Comments:
@louispotok : Thanks for doing this!  Before we can merge, you will need to add tests and `whatsnew` entry to this PR.
needs tests that validate chunksize is current and errors correctly

see how read_csv does this
@jreback thanks for the tip. I looked at the read_csv tests and I'm still a bit confused. Specifically I'm looking at "test_read_chunksize" in tests/io/parser/common.py. I see that it's testing two main things:
1) Each returned chunk is equal to that slice of the input dataframe -- not relevant in this case, since read_json is still returning the whole dataframe.
2) Ensure that read_csv is using _validate_integer. I wasn't using that on read_json, but I added it  and am now testing for it.
Okay, the latest commits should address all these comments:

- describe change in whatsnew
- add version-added tag to the chunksize param in the read_json docstring
- add docstring for _read_json_as_lines internal function
- validate that chunksize is integer-like and >=1
- add tests for chunksize: reads correctly and rejects invalid chunksize argument.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@9b07ef4`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17168/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #17168   +/-   ##
=========================================
  Coverage          ?   90.97%           
=========================================
  Files             ?      162           
  Lines             ?    49500           
  Branches          ?        0           
=========================================
  Hits              ?    45035           
  Misses            ?     4465           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.75% <100%> (?)` | |
| #single | `40.26% <0%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `100% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=footer). Last update [9b07ef4...70a43a1](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=h1) Report
> Merging [#17168](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/cc58b84f423db58de9edca762f0abbe10c638efb?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17168/graphs/tree.svg?token=eZ4WkYLtcO&src=pr&height=150&width=650)](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17168      +/-   ##
==========================================
- Coverage   91.27%   91.25%   -0.02%     
==========================================
  Files         163      163              
  Lines       49766    49766              
==========================================
- Hits        45423    45414       -9     
- Misses       4343     4352       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.05% <ø> (ø)` | :arrow_up: |
| #single | `40.34% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `100% <ø> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.73% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=footer). Last update [cc58b84...28d1cbe](https://codecov.io/gh/pandas-dev/pandas/pull/17168?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Also, do you prefer to leave these atomic commits or should I squash them?
@louispotok : Whatever is most convenient for you.  We'll squash them at the end once we merge.
Thanks @gfyoung , this is ready for another round of review.
@louispotok : This looks pretty good (there are some minor stylistic things I think...@jreback?)

Could you post timing information so that we can see the performance difference with and without your changes in this PR?

Link: https://pandas.pydata.org/pandas-docs/stable/contributing.html#id53
My main comment/question here is: should the API be consistent with how `chunksize` is for `read_csv` ? 
This would mean that when using this, the return value is an iterator.

@louispotok I understand that you don't need it in your specific case (as once concatenated, your dataframe fits into memory). But in general it could be needed in certain cases to have it as an iterator. For your case you can then do the concat yourself:

```
In [41]: s = """a,b
    ...: 1,2
    ...: 3,4
    ...: 5,6
    ...: 7,8"""

In [42]: pd.read_csv(StringIO(s))
Out[42]: 
   a  b
0  1  2
1  3  4
2  5  6
3  7  8

In [43]: pd.read_csv(StringIO(s), chunksize=2)
Out[43]: <pandas.io.parsers.TextFileReader at 0x7f2e9c2ef630>

In [44]: pd.concat(pd.read_csv(StringIO(s), chunksize=2))
Out[44]: 
   a  b
0  1  2
1  3  4
2  5  6
3  7  8
```
It looks like we still need a decision on whether to return a `BaseIterator` or just yield chunks. I don't have a strong opinion on that and will wait for a decision there.

I'll point out one thing though. I think there are really two issues here worth separating:
1) The existing implementation doesn't permit loading JSONs even when the full dataframe will fit in memory (this was the original bug report).
2) Should we provide a way for users to handle JSON dataframes that *don't* fit in memory by returning an iterator / generator?

I was originally attempting to fix (1) but there may be another way to fix it instead. The culprit seems to be [line 349](https://github.com/pandas-dev/pandas/blob/master/pandas/io/json/json.py#L349) which on my laptop kills 10G of available memory with a 1.7GB json object. My solution to (1) was to read in the file in chunks, but maybe there are other possible fixes.  Then, I think a separate pull request could solve issue (2). 

What do you think?
> It looks like we still need a decision on whether to return a BaseIterator or just yield chunks. I don't have a strong opinion on that and will wait for a decision there.

make a class that inherits BaseIterator, call it JsonLineReader. return this to the user. this solves both issues generally.
Hello @louispotok! Thanks for updating the PR.

 - In the file [`asv_bench/benchmarks/io_bench.py`](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py), following are the PEP8 issues :

> [Line 205:80](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py#L205): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (97 > 79 characters)
> [Line 206:35](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py#L206): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ','
> [Line 206:52](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py#L206): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ','
> [Line 211:9](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py#L211): [E722](https://duckduckgo.com/?q=pep8%20E722) do not use bare except'
> [Line 218:72](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py#L218): [E226](https://duckduckgo.com/?q=pep8%20E226) missing whitespace around arithmetic operator
> [Line 224:72](https://github.com/pandas-dev/pandas/blob/28d1cbe6cefa4561d79c8cf6245f3448b4f5b422/asv_bench/benchmarks/io_bench.py#L224): [E226](https://duckduckgo.com/?q=pep8%20E226) missing whitespace around arithmetic operator









##### Comment last updated on September 28, 2017 at 21:47 Hours UTC
*** Note: not ready for re-review -- found some bugs that were not caught by tests. Fixing behavior and tests.
Okay, this is ready for re-review. 

@jreback I believe I've addressed all your comments.
@gfyoung In my latest (coarse) performance testing, with large enough chunks this is not noticeably slower than reading it all at once. Sorry for the false alarm.

One other point I want to confirm. I'm assuming (based on my reading of the code and documentation) that currently when `lines=True`, we always have `orient="records"` and the resulting object should always have an integer index `[0, 1, ... , len(obj) - 1 ]`. Is that a true assumption?
I'm not sure why the AppVeyor build broke, it looks like an unrelated issue.
@louispotok : Maybe not.  `ResourceWarning` could be indicative of a stream not having been properly closed, which is related to what you are doing.
Ah, I think I caught it. Let's see. To document fully, in my understanding, there are 4 cases to consider. Here's the current behavior:
1. filepath passed in without chunksize: file is opened, then read in, then closed (not changed from previous)
2. filepath passed in with chunksize: file is opened, then JsonLineReader is returned. When JsonLineReader gets to the end of the file, it attempts to close the file.
3. Buffer passed in without chunksize: json is read out of the buffer using filepath_or_buffer.read(), and then the object is returned, but the buffer stays open. (unchanged from previous)
4. Buffer passed in with chunksize: JsonLineReader is returned and assumes that buffer will stay open until it has finished iterating through. When it is done, it closes the buffer.

Do those decisions make sense? I could see the argument for changing (4) so that it leaves the buffer open and the caller has to close it.

I think (and we'll see with this build) the problem was in situation (2), I was checking that chunksize was valid *after* the file was opened, so the test suite correctly caught the error (`assert_raises_regex`) but was then exiting while leaving the file open. If that's true, we'll have to find a way to write a test for this.
Hm, still failing and I'm not sure how to investigate further. I do notice that the method that errors (`assert_produces_warning`) says it is not thread safe, not sure if that is relevant. Would appreciate some help here figuring out how to fix this.
@gfyoung 
I'm unable to reproduce this locally. If I were able to, I could try to trace it better (as in [here](https://docs.python.org/3/whatsnew/3.6.html?highlight=resourcewarning#warnings)) but I'm stuck. 

I've tried a number of things to get to the bottom of this but no luck:
1) Run the full test suite locally -- can't reproduce
2) Run the full test suite locally multithreaded as described [here](https://pandas.pydata.org/pandas-docs/stable/contributing.html#running-the-test-suite) -- can't reproduce
3) Created a test script to leave a bunch of files open (with `warnings.default_action = "error"`). This causes an IOError with ~1000 files open, but no ResourceWarning.
4) Created a test script to leave a bunch of buffers open (`StringIO` specifically, with `warnings.default_action = "error"), got into millions without a ResourceWarning.

I'd love any ideas or thoughts about how to proceed. Thanks!
@louispotok : Sorry about that!  I think I missed your message in the sea of messages that I get daily 😄 .  I would suggest that you try rebasing onto `master` and see whether or not tests still fail.

I must say that it is kind of weird that this test was already failing from your first commit.  If AppVeyor still breaks, take that failing test and walk through the code using your changes from the your first commit *only* to see what's going on.
Great! @gfyoung I rebased to master and AppVeyor passes now. But the Travis CI build terminates because no output received in 10minutes, which again does not seem like a problem due to this change. WDYT?
The OSX builds are generally a little more annoying on Travis.  I'll restart, as it terminates well-before it even builds `pandas`.
yeah I here other folks (arrow) are having a similar Travis with macosx today.... 

cc @wesm 
@jreback Thanks for the review. I incorporated some of your comments. Two places I diverged:
1. Using a function instead of a fixture for the test data, since I need a StringIO and if I use a fixture I can only read it once per test.
2. I didn't make the suggested change around opening the file within `JsonLineReader`, explanation [here](https://github.com/pandas-dev/pandas/pull/17168#discussion_r137959567).
@jreback This is ready for another look whenever you have a chance.
@jreback Thanks for the helpful review. New changes:
* updated the docs
* use JsonReader in both cases (and just `return JsonReader.read()` when appropriate)
* Created the test fixture and removed the other testing helper function as per request

Two things you asked that I wasn't sure about:
* Moving close to its own method -- happy to do it if you like, but is there another advantage to it?
* I parameterized the tests in two places by just using a for loop. Given what I'm doing and how the fixture is constructed I'm not sure how using pytests fixture parametrization would actually work here, so if you had something else in mind I'd appreciate a more explicit instruction.
> Moving close to its own method -- happy to do it if you like, but is there another advantage to it?

just hygiene
@chris-b1 can you have a look.