You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/05 21:30:14 UTC
[GitHub] [arrow-rs] andygrove opened a new pull request, #3468: Improve CSV error messages
andygrove opened a new pull request, #3468:
URL: https://github.com/apache/arrow-rs/pull/3468
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3467
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Better UX
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Improve error message
# Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#issuecomment-1372807561
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] andygrove commented on pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#issuecomment-1372825101
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#discussion_r1062927669
##########
arrow-csv/src/reader/mod.rs:
##########
@@ -1124,6 +1124,31 @@ mod tests {
use chrono::prelude::*;
+ #[test]
+ fn rows_unequal_length() {
+ let file = File::open("test/data/unequal_lengths.csv").unwrap();
+ let schema = Schema::new(vec![
+ Field::new("foo", DataType::Int32, false),
+ Field::new("bar", DataType::Int32, false),
+ ]);
+ let mut csv = Reader::new(
+ file,
+ Arc::new(schema.clone()),
+ false,
+ None,
+ 1024,
+ None,
+ None,
+ None,
+ );
+ let batch = csv.next().unwrap();
+ assert!(batch.is_err());
+ assert_eq!(
+ "Csv error: incorrect number of fields, expected 2 got 3",
Review Comment:
This logic is only used by the inference logic (which FWIW desperately needs some TLC), not the reader
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#issuecomment-1372832794
I'll see if I can't quickly bash something out for you, as I've touched this code recently
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] andygrove commented on pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#issuecomment-1372802479
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#discussion_r1062927669
##########
arrow-csv/src/reader/mod.rs:
##########
@@ -1124,6 +1124,31 @@ mod tests {
use chrono::prelude::*;
+ #[test]
+ fn rows_unequal_length() {
+ let file = File::open("test/data/unequal_lengths.csv").unwrap();
+ let schema = Schema::new(vec![
+ Field::new("foo", DataType::Int32, false),
+ Field::new("bar", DataType::Int32, false),
+ ]);
+ let mut csv = Reader::new(
+ file,
+ Arc::new(schema.clone()),
+ false,
+ None,
+ 1024,
+ None,
+ None,
+ None,
+ );
+ let batch = csv.next().unwrap();
+ assert!(batch.is_err());
+ assert_eq!(
+ "Csv error: incorrect number of fields, expected 2 got 3",
Review Comment:
This logic is only used by the inference logic, not the reader
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] andygrove commented on a diff in pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #3468:
URL: https://github.com/apache/arrow-rs/pull/3468#discussion_r1062926741
##########
arrow-csv/src/reader/mod.rs:
##########
@@ -1124,6 +1124,31 @@ mod tests {
use chrono::prelude::*;
+ #[test]
+ fn rows_unequal_length() {
+ let file = File::open("test/data/unequal_lengths.csv").unwrap();
+ let schema = Schema::new(vec![
+ Field::new("foo", DataType::Int32, false),
+ Field::new("bar", DataType::Int32, false),
+ ]);
+ let mut csv = Reader::new(
+ file,
+ Arc::new(schema.clone()),
+ false,
+ None,
+ 1024,
+ None,
+ None,
+ None,
+ );
+ let batch = csv.next().unwrap();
+ assert!(batch.is_err());
+ assert_eq!(
+ "Csv error: incorrect number of fields, expected 2 got 3",
Review Comment:
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 ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] andygrove closed pull request #3468: Improve CSV error messages
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #3468: Improve CSV error messages
URL: https://github.com/apache/arrow-rs/pull/3468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org