Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve CSV error messages #3468

Closed

Conversation

andygrove
Copy link
Member

Which issue does this PR close?

Closes #3467

Rationale for this change

Better UX

What changes are included in this PR?

Improve error message

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jan 5, 2023
@andygrove
Copy link
Member Author

These changes did not break any tests, therefore there are no tests ...

@alamb @tustvold I am not familiar with this repo and could use guidance on best place to add tests for this change

@tustvold
Copy link
Contributor

tustvold commented Jan 5, 2023

FWIW I think this logic is only used by the schema inference logic, following #3365 the core parsing logic uses a different code path.

best place to add tests for this change

A unit test within the module that produces the error, in the case of this PR currently reader, but potentially records if you want to cover that also

let batch = csv.next().unwrap();
assert!(batch.is_err());
assert_eq!(
"Csv error: incorrect number of fields, expected 2 got 3",
Copy link
Member Author

Choose a reason for hiding this comment

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

No line number is included here .. I wonder what is needed to have the csv crate populate the position in the error? I guess I have some research to do ...

Copy link
Contributor

@tustvold tustvold Jan 5, 2023

Choose a reason for hiding this comment

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

This logic is only used by the inference logic (which FWIW desperately needs some TLC), not the reader

@andygrove
Copy link
Member Author

FWIW I think this logic is only used by the schema inference logic, following #3365 the core parsing logic uses a different code path.

ok, thanks .. will close this for now then

@andygrove andygrove closed this Jan 5, 2023
@tustvold
Copy link
Contributor

tustvold commented Jan 5, 2023

I'll see if I can't quickly bash something out for you, as I've touched this code recently

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.

Include position in CSV error messages
2 participants