Detect Parsing errors in read_csv first row with index_col=False
State: closed

- [X] closes #40333
- [X] closes #22144
- [x] tests added / passed
- [X] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [X] whatsnew entry


Labels:
name: Bug
name: Error Reporting
description: Incorrect or improved errors from pandas
name: IO CSV
description: read_csv, to_csv

Comments:
> Thanks for the pr @njriasan! Can you revert the unrelated changes - it makes it harder to review since it complicates the diff.

Hi @mzeitlin11. I reverted the unrelated changes. Sorry for the delay.
I think there may be a logical error in my PR based upon knowledge gaps in my Pandas understanding. I'll investigate any failed tests tomorrow and I may ask for further clarification if possible.
@mzeitlin11 I took a look at the tests that were failing and attempted to fix them. I have one question regarding `test_no_header_two_extra_columns`. Based upon the linked github issue: https://github.com/pandas-dev/pandas/issues/26218, it seems like the intended behavior is to throw an exception when the number of columns doesn't match the number of names. My change fails this test, but it now provides that error for the Python parser. I understand the C parser should be consistent (I believe the change should be simple and I just need to initialize the number of expected fields to the number of names), but I wanted to confirm that my understanding was correct before proceeding. 
> @mzeitlin11 I took a look at the tests that were failing and attempted to fix them. I have one question regarding `test_no_header_two_extra_columns`. Based upon the linked github issue: #26218, it seems like the intended behavior is to throw an exception when the number of columns doesn't match the number of names. My change fails this test, but it now provides that error for the Python parser. I understand the C parser should be consistent (I believe the change should be simple and I just need to initialize the number of expected fields to the number of names), but I wanted to confirm that my understanding was correct before proceeding.

Sorry @njriasan, not too familiar with this part of the codebase. Definitely agree on the consistency point, but not sure about throwing an exception. Especially in the case of `error_bad_lines=False`, if the case wasn't previously raising, we probably don't want to make it start raising without a deprecation cycle. Maybe this pr can just focus on the `error_bad_lines=True` case, for which I think we'd definitely want to raise.
> > @mzeitlin11 I took a look at the tests that were failing and attempted to fix them. I have one question regarding `test_no_header_two_extra_columns`. Based upon the linked github issue: #26218, it seems like the intended behavior is to throw an exception when the number of columns doesn't match the number of names. My change fails this test, but it now provides that error for the Python parser. I understand the C parser should be consistent (I believe the change should be simple and I just need to initialize the number of expected fields to the number of names), but I wanted to confirm that my understanding was correct before proceeding.
> 
> Sorry @njriasan, not too familiar with this part of the codebase. Definitely agree on the consistency point, but not sure about throwing an exception. Especially in the case of `error_bad_lines=False`, if the case wasn't previously raising, we probably don't want to make it start raising without a deprecation cycle. Maybe this pr can just focus on the `error_bad_lines=True` case, for which I think we'd definitely want to raise.

So this PR should also fix https://github.com/pandas-dev/pandas/issues/22144, because the changes to the Python version fix this PR as well (so I modified the C code to be consistent). This does require reverting the test for https://github.com/pandas-dev/pandas/issues/26218 to raise an exception, which based on the comments in the issue and 22144 is the desired behavior.

This should not raise an error in the case of `error_bad_lines=False`, but I can add an extra test to check this explicitly if you would like. I understand it's probably worse practice to try and fix both issues in 1 PR, but I'm not sure its possible to cleanly decouple these issues in the Python parser.
@mzeitlin11 can you give me some advice on how to determine the build errors I'm getting on the tests? Everything runs locally on my machine and I haven't modified the failing module at all. Perhaps my issues are because I had to fix a merge issue in the `whatsnew` file?
> @mzeitlin11 can you give me some advice on how to determine the build errors I'm getting on the tests? Everything runs locally on my machine and I haven't modified the failing module at all. Perhaps my issues are because I had to fix a merge issue in the `whatsnew` file?


Looks like a build failure. For example in https://github.com/pandas-dev/pandas/pull/40629/checks?check_run_id=2376337837 seeing
```
pandas/_libs/src/parser/tokenizer.c: In function ‘end_line’:
334
pandas/_libs/src/parser/tokenizer.c:451:50: error: comparison of integer expressions of different signedness: ‘uint64_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
335
  451 |         !((self->skip_header_end && (self->lines < self->allow_leading_cols))
336
      |                                                  ^
337
cc1: all warnings being treated as errors
```
You might not see this locally if compile flags are different so that certain warnings don't error locally
> > @mzeitlin11 can you give me some advice on how to determine the build errors I'm getting on the tests? Everything runs locally on my machine and I haven't modified the failing module at all. Perhaps my issues are because I had to fix a merge issue in the `whatsnew` file?
> 
> Looks like a build failure. For example in https://github.com/pandas-dev/pandas/pull/40629/checks?check_run_id=2376337837 seeing
> 
> ```
> pandas/_libs/src/parser/tokenizer.c: In function ‘end_line’:
> 334
> pandas/_libs/src/parser/tokenizer.c:451:50: error: comparison of integer expressions of different signedness: ‘uint64_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
> 335
>   451 |         !((self->skip_header_end && (self->lines < self->allow_leading_cols))
> 336
>       |                                                  ^
> 337
> cc1: all warnings being treated as errors
> ```
> 
> You might not see this locally if compile flags are different so that certain warnings don't error locally

Thanks @mzeitlin11 this fixed it! All the tests pass now. Can you let me know what are the next steps to get this ready to merge?
> Thanks @mzeitlin11 this fixed it! All the tests pass now. Can you let me know what are the next steps to get this ready to merge?

More rounds of review from others :)
cc @phofl just noticed #38587 if you have any thoughts on approach here
Will look more closely later, but I don't think we should change the behavior without deprecating first. Based on work experience when working with proprietary systems this is not that unusual unfortunately.

In #38587 we weren't even sure if we would like to deprecate or just warn
Hi I'm just wondering if this feature change needs to go through a `deprecation` change, what are the next steps for me?
Two options here:

If you can fix the bug without breaking the test, you can do this here and open up another pr raising a FutureWarning for the behaviro which is tested but looks wrong.

If you can not fix this without breaking the test, you could raise a FutureWarning in this case and put off the pr to submit for 2.0
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.
@njriasan still working on this? if so pls merge master
Thanks for the PR, but it appears to have gone stale. If you'd like to continue, please merge master, and address the the comments (around the `FutureWarning`), and we can reopen