Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ENH: add arrow engine to read_csv #31817
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
ENH: add arrow engine to read_csv #31817
Changes from 7 commits
f22ff46
8ae43e4
09074df
6be276d
df4fa7e
9cd9a6f
ecaf3fd
b3c3287
474baf4
2cd9937
48ff255
3d15a56
c969373
98aa134
b9c6d2c
67c5db6
7f891a6
11fc737
23425f7
d9b7a1f
b8adf3c
01c0394
ba5620f
2570c82
b3a1f66
d46ceed
d67925c
6378459
9d64882
852ecf9
93382b4
f1bb4e2
14c13ab
7876b4e
4426642
008acab
2dddae7
261ef6a
88e200a
bf063ab
ede2799
e8eff08
87cfcf5
55139ee
c1aeecf
62fc9d6
b53a620
f13113d
f9ce2e4
4158d6a
d34e75f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could do this at the top of the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that
nrows
is not supported by pyarrow, so in principle we should only need to validate it for other engines.Now, I suppose that before you end up here, we already would have raised an error if
nrows
was specified in case of the pyarrow engine, so it probably doesn't hurt to needlessly validate the defaultnrows
argument in case of the pyarrow engine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess I could do that, but I think my way is cleaner, since all the pyarrow code would be in the if, and the other parser code would be in the else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will need to refactor this as the current code is very different from this.
Also I really don't like doing all of this validation in a single function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify a bit more what you mean? Or point to recent changes related to this?
For example also on master, the C parser is using a very similar mechanism with the CParserWrapper class.
Or do you only mean that it needs to split some validation into separate methods (as you indicate in a comment below as well, fully agreed with that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls add line breaks between section and comments.