BUG: csv parsing with EOF byte on windows, Revert gh-16039
State: closed

#16039 created a bug in which files containing byte-like data could break on Windows, as EOF characters mid-field (despite being quoted) would cause premature line breaks.  Given that this PR was a performance patch, this commit can be safely reverted.

Closes #16559 (confirmed with unit test).

cc @dgwynne 

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

Comments:
looks reasonable. 3.6/windows is failing & pls run the csv asv's just to confirm no regression.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=h1) Report
> Merging [#16663](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/18a428da81f7fe0f41f8ec78f03a385b731bb6a3?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #16663      +/-   ##
==========================================
+ Coverage   90.93%   90.96%   +0.02%     
==========================================
  Files         161      161              
  Lines       49269    49266       -3     
==========================================
+ Hits        44802    44813      +11     
+ Misses       4467     4453      -14
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.72% <ø> (+0.02%)` | :arrow_up: |
| #single | `40.22% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.36% <0%> (+0.09%)` | :arrow_up: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.05% <0%> (+1.81%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16663?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/16663?src=pr&el=footer). Last update [18a428d...045966a](https://codecov.io/gh/pandas-dev/pandas/pull/16663?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/16663?src=pr&el=h1) Report
> Merging [#16663](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/18a428da81f7fe0f41f8ec78f03a385b731bb6a3?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #16663      +/-   ##
==========================================
- Coverage   90.93%   90.93%   -0.01%     
==========================================
  Files         161      161              
  Lines       49269    49270       +1     
==========================================
  Hits        44802    44802              
- Misses       4467     4468       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.69% <100%> (-0.01%)` | :arrow_down: |
| #single | `40.22% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/parsers.py](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhcnNlcnMucHk=) | `95.43% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.23% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16663?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/16663?src=pr&el=footer). Last update [18a428d...69a4ad0](https://codecov.io/gh/pandas-dev/pandas/pull/16663?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> 3.6/windows is failing

Actually, it's the 2.7 build on Windows (it passes just fine on Python 3.6)

> pls run the csv asv's just to confirm no regression.

Didn't see any noticeable regressions on my end.
@jreback : All green and ready to go now.
@jreback : I think you raise a couple of good points, but I think I would prefer to merge this (so that the bug is patched) and look into those afterwards as a follow-up, if that works?
> I think you raise a couple of good points, but I think I would prefer to merge this (so that the bug is patched) and look into those afterwards as a follow-up, if that works?

that's fine. marking for 0.20.3 as this is a bug fix. if you run into issues and we have to make a bigger change can always just move to 0.21

yes bullet-proofing memory is an issue here (though hard to nail these types of things down).
hi guys, sorry im late to the party.

i am surprised by this, i thought that EOF was supposed to be represented outside the allowed set of characters you can have in a file, but after discussing this with a few people i get the impression that windows is special. as suggested by bug 5500, EOF on windows is 0x1a or 27, which can definitely end up in a file. on unix-like systems EOF tends to map to -1 which is supposed to outside what a char can represent.

one of the people i talked to pointed out that i dropped the binary flag when moving from fopen() ("b" in the mode string) to open(). in unix-like systems, the b flag is meaningless, but not on windows.

from what i can tell, ORing O_BINARY into the flags to open() might be enough to fix the problem. because O_BINARY may not be provided on other platforms, a common pattern for handling that is to put something like the following toward the top of the file:

```
#ifndef O_BINARY
#define O_BINARY 0
#endif /* O_BINARY */
```
@dgwynne : 

> windows is special

That's one way to put it. 😉 

> from what i can tell, ORing O_BINARY into the flags to open() might be enough to fix the problem

Hmm...interesting.  Feel free to give it a shot.  Then we can re-apply your PR with that patch if it works.
i don't have a windows box with a toolchain i can test with. if i generate a change against head, would you (or someone) be willing to try it?
We test the codebase against Appveyor, which runs Windows boxes.  You can create an account for free, hook it up to your GitHub account, and it will run those tests for you before you submit the PR (or you could just submit immediately and let the tests run).
is there a test case that will catch this problem now?
Yes, I added one in this PR.
#16675 has been created. do i just wait now?