Skip to content

Conversation

@psiayn
Copy link
Contributor

@psiayn psiayn commented Mar 31, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

This change allows for better error messages in the arrow-csv crate as suggested in the issue for ease of debugging. Row number is still for a batch not the csv file, so it shows the entire row.

Are there any user-facing changes?

The changed don't affect public APIs.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Mar 31, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR @psiayn ❤️

The one thing I think it needs are some tests (of the error messages)

One way to do that would be to try and parse some CSV data, and then calling unwrap_err().to_string() to convert the result into a string and assert_eq! that it matches

FYI @niebayes

@psiayn
Copy link
Contributor Author

psiayn commented Mar 31, 2025

Sure! I'll add tests which cover more cases, I noticed that the tests weren't really handling all the errors but thought of adding that as another enhancement.

@alamb
Copy link
Contributor

alamb commented Mar 31, 2025

I think it would be very valuable to add tests -- if you want to make a separate PR first to add error tests that would be great too.

@psiayn
Copy link
Contributor Author

psiayn commented Apr 1, 2025

I will add it in this PR itself

@alamb
Copy link
Contributor

alamb commented Apr 1, 2025

Thanks @psiayn !

@alamb alamb marked this pull request as draft April 1, 2025 16:25
@alamb
Copy link
Contributor

alamb commented Apr 1, 2025

Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look

@alamb
Copy link
Contributor

alamb commented Apr 4, 2025

❤️ -- I kicked off the CI checks on this PR too

@alamb alamb marked this pull request as ready for review April 4, 2025 15:25
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @psiayn -- very nice contribution

@alamb
Copy link
Contributor

alamb commented Apr 4, 2025

FYI @niebayes

@alamb alamb merged commit cf27032 into apache:main Apr 4, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve CSV parsing errors: Print the row that makes csv parsing fails

2 participants