Skip to content

ARROW-10677: [Rust] Fix CSV Boolean parsing + add tests to demonstrate supported csv parsing #8733

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.

Sign up for GitHub

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

Jump to bottom
Closed
alamb wants to merge 1 commit into from
Closed

ARROW-10677: [Rust] Fix CSV Boolean parsing + add tests to demonstrate supported csv parsing #8733

alamb wants to merge 1 commit into from

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Nov 21, 2020 •
edited
Loading

Background:
https://github.com/apache/arrow/pull/8714/files# / ARROW-10654 adds some specialized parsing for the csv reader but there was no unit test coverage.

I am fairly sure that a late commit actually introduced a bug, but clearly there weren't unit tests to find it (see details 52bf0e8#r528201732)

Changes:

  1. Fix the bug
  2. Add boolean parsing support as protection against regression and explicit documentation of what boolean types are supported so that if we decide to change these implementations in the future it will be clearer if we are adding/removing support

@github-actions
Copy link

github-actions bot commented Nov 21, 2020

https://issues.apache.org/jira/browse/ARROW-10677

@alamb alamb mentioned this pull request Nov 21, 2020
Closed
@alamb alamb force-pushed the alamb/ARROW-10677-boolean-parsing branch from 3a3a353 to 3f63f67 Compare November 21, 2020 23:12
@alamb alamb marked this pull request as ready for review November 21, 2020 23:12
@Dandandan
Copy link
Contributor

Dandandan commented Nov 22, 2020

👍

@alamb alamb force-pushed the alamb/ARROW-10677-boolean-parsing branch from 3f63f67 to 0432192 Compare November 22, 2020 12:24
alamb
alamb commented Nov 22, 2020
View reviewed changes
rust/arrow/src/csv/reader.rs Outdated
Copy link
Contributor Author

@alamb alamb Nov 22, 2020

Choose a reason for hiding this comment

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

note the condition was inverted here ("False" --> Some(true)) -- this was introduced in #8714 by accident

@alamb alamb changed the title ARROW-10677: [Rust] Add tests to demonstrate supported csv parsing ARROW-10677: [Rust] Fix CSV Boolean parsing + add tests to demonstrate supported csv parsing Nov 22, 2020
@alamb alamb requested a review from nevi-me November 22, 2020 12:29
@alamb
Copy link
Contributor Author

alamb commented Nov 23, 2020

looks like this needs a rebase -- doing so now

@alamb alamb force-pushed the alamb/ARROW-10677-boolean-parsing branch from 0432192 to 67cd72a Compare November 23, 2020 21:44
@alamb alamb closed this in 42564e4 Nov 23, 2020
@alamb alamb deleted the alamb/ARROW-10677-boolean-parsing branch December 7, 2020 19:30
@asfimport asfimport mentioned this pull request Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dandandan Dandandan Dandandan approved these changes

@paddyhoran paddyhoran paddyhoran approved these changes

@nevi-me nevi-me Awaiting requested review from nevi-me

+1 more reviewer

@vertexclique vertexclique vertexclique approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alamb @Dandandan @vertexclique @paddyhoran