Allow parsing in skipped row for C engine
State: closed

Changes behaviour of C engine parser so that parsing is done on skipped rows so that they
are properly skipped.

Closes #10911.
Closes #12775.


Labels:
name: Bug
name: IO CSV
description: read_csv, to_csv

Comments:
The issue with allowing parsing on skipped rows is that there is a performance hit since we're spending more time processing rows that don't really matter.  How significant is it though do you think?

this for sure needs performance testing. You are allowing a special case (that of a terminator character IN the skiprows) to influence the workhorse parsing. I am not sure this is the correct soln.

@jreback : it's actually a little more general than that.  I am addressing all of the cases that occur in `tokenizer.c`, which goes beyond just newline characters in quoted fields bug.  The other option is to add new logic in the `SKIP_LINE` state to just check for quotes but I wasn't sure about how it would handle quotes within quotes?

well, you can run the benchmarks in the test suite and see what shows up. prob need some addtl ones that hae embeeded terminators and such. I would special case thinks for a situation like this.

"special case thinks" ?

I think

Added some additional unit tests that my current fixes should be robust against.  Let me know if there are any additional unit tests that should be included/written.

Also, which benchmarks should I run my changes against?  I had some difficulty finding relevant benchmarks in the code base.

@gfyoung There is one specific asv bench for skiprows (io_bench/read_csv_skiprows), but you can eg run all tests that contain `read_csv`, or all tests in the io_bench.py file

@jorisvandenbossche : how do I exactly run only one of the benchmark tests (i.e. just `io_bench.py`)?  The `vb_suite/run_suite.py` file doesn't seem to have any way to configure which benchmarks to run.

See the contributing docs on performance testing: http://pandas-docs.github.io/pandas-docs-travis/contributing.html#running-the-performance-test-suite (if anything is not clear, please indicate so we can improve the explanation)

@jorisvandenbossche : Thank you!  That link was very helpful.  I was looking on the `github.io` page for `vbench`, which was not as detailed as this.

<b>@Everyone</b>: I ran the benchmark for `io_bench.csv_skiprows`.  `master` currently runs it in 18.62 ms, while my PR runs it in 26.76 ms.

pls run all of the csv benchmarks 

|  | master (ms) | PR (ms) |
| --- | :-: | --: |
| time_frame_to_csv | 220.30 | 227.45 |
| time_frame_to_csv2 | 377.33 | 364.85 |
| time_frame_to_csv_date_formatting | 17.01 | 15.51 |
| time_frame_to_csv_mixed | 269.36 | 257.73 |
| time_read_csv_infer_datetime_format_custom | 18.10 | 18.30 |
| time_read_csv_infer_datetime_format_iso8601 | 2.97 | 3.10 |
| time_read_csv_infer_datetime_format_ymd | 3.30 | 3.21 |
| time_read_csv_skiprows | 18.23 | 25.74 |
| time_read_csv_standard | 14.95 | 15.65 |
| time_read_parse_dates_iso8601 | 2.05 | 2.14 |
| time_write_csv_standard | 58.79 | 59.01 |

As can be observed according to `asv`, the performance has not been significantly impacted by my changes AFAICT.

If there are no complaints about the performance or the changes (they pass currently against Travis, I added `[no skip]` to change commit message), this PR is ready to merge.

pls have a look thru any open csv parsing issues. you changed some things which may allow some of them to be fixed e.g. things related to lineterminator and such.

I took a look through all of the issues, and I think the two that I mentioned so far are the only ones that can be closed with this PR.

https://github.com/pydata/pandas/search?q=csv+lineterminator&type=Issues&utf8=%E2%9C%93

pls have a look at these and see if any are relevant

@jreback : I looked through all of the open issues tagged with `CSV` before I made that comment above.  

FYI, you also should have received emails about several of the older `CSV` issues that either should be closed, except for one that I think can be patched.

@jreback : if there is nothing else, I think this is ready to merge.

For the skiprows benchmark, there is a considerable slowdown, which is of course logical since there is now more parsing, and the exact slowdown of course depends on the number of skipped rows (this is just the consequence of wanting to solve the bug). 
But the question is a bit of we are OK with this performance hit for such a corner case. I can imagine people use `skiprows` exactly to have a faster parsing when they need only part of the file? 
However, I also can't think of a nice way around this (apart from yet another keyword ..).

@jorisvandenbossche : Yes, that slowdown seems to be an unfortunate necessity at this point for the sake of correctness.  As I mentioned earlier, the other option was to add more logic under the `SKIP_LINE` block, but attempts to do so were not robust against additional testing and only appeared to duplicate existing code.

@jorisvandenbossche , @jreback : I'm going to table this one for now until #12939 gets merged in.  It will be a lot easier to merge a fix for this with only one (hopefully) tokenizing function to work with.

I also realized that an issue with my current fix is that we will be penalizing people if the fields in the skipped row are malformed (since we are parsing them as if they were rows to be included), which is not what we want IINM.  Thus, I might need to go back to the drawing board (even if the correctness > the performance as the PR currently stands) to see whether I can build in such safety mechanisms or go back to my other strategy of introducing some additional states for skipped rows.

I knew there had to be a better way.  Just needed some fresh eyes on the issue and managed to get the adding of a couple of new states to work --> there won't be any performance hit now like before.  Let's see what Travis thinks.

ok pls rerun the perf tests as well (and maybe run vs 0.18.0) as a comparison 

# PR Branch

<pre>io_bench.frame_to_csv.time_frame_to_csv           212.25ms
io_bench.frame_to_csv2.time_frame_to_csv2         356.49ms
...formatting.time_frame_to_csv_date_formatting    15.38ms
...h.frame_to_csv_mixed.time_frame_to_csv_mixed   253.26ms
...m.time_read_csv_infer_datetime_format_custom    18.20ms
...time_read_csv_infer_datetime_format_iso8601     3.00ms
..._ymd.time_read_csv_infer_datetime_format_ymd     3.21ms
...nch.read_csv_skiprows.time_read_csv_skiprows    20.18ms
...nch.read_csv_standard.time_read_csv_standard    16.00ms
..._dates_iso8601.time_read_parse_dates_iso8601     2.07ms
...h.write_csv_standard.time_write_csv_standard    59.00ms</pre>

# Master Branch

<pre>io_bench.frame_to_csv.time_frame_to_csv           216.67ms
io_bench.frame_to_csv2.time_frame_to_csv2         353.74ms
...formatting.time_frame_to_csv_date_formatting    15.28ms
...h.frame_to_csv_mixed.time_frame_to_csv_mixed   254.94ms
...m.time_read_csv_infer_datetime_format_custom    17.82ms
....time_read_csv_infer_datetime_format_iso8601     3.03ms
..._ymd.time_read_csv_infer_datetime_format_ymd     3.16ms
...nch.read_csv_skiprows.time_read_csv_skiprows    19.74ms
...nch.read_csv_standard.time_read_csv_standard    16.08ms
..._dates_iso8601.time_read_parse_dates_iso8601     2.11ms
...h.write_csv_standard.time_write_csv_standard    59.25ms</pre>

FYI (tangent): I think you have an old build from your most recent PR that you just merged running on Travis right now.

@jreback : Travis is giving the green light, and I don't see any major timing discrepancies.  Ready to merge if there is nothing else.

thanks!