-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47575: [Python] add quoting_header option to pyarrow WriterOptions #47610
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
GH-47575: [Python] add quoting_header option to pyarrow WriterOptions #47610
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
|
|
pitrou
left a comment
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.
Thank you @ayushbansal07 . This looks mostly good to me, just two suggestions to improve the documentation.
|
|
||
| def __init__(self, *, include_header=None, batch_size=None, | ||
| delimiter=None, quoting_style=None): | ||
| delimiter=None, quoting_style=None, quoting_header=None): |
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'll need to add a doc for the new argument in the class docstring above as well.
python/pyarrow/_csv.pyx
Outdated
| @property | ||
| def quoting_header(self): | ||
| """ | ||
| Same as quoting_style, but for header column names |
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.
I think we should add the note that is found in the C++ docs, as otherwise people may be surprised by the behavior.
|
Thanks for the suggestions @pitrou. Have made the specified changes. |
|
@AlenkaF Do you want to give this a look? |
AlenkaF
left a comment
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.
Thank you for the contribution @ayushbansal07!
Looks good to me, I only have one minor comment.
|
@github-actions crossbow submit -g python |
|
Will just wait for the CI and the extended builds and will merge if all looks ok. |
|
Revision: ae57525 Submitted crossbow builds: ursacomputing/crossbow @ actions-2a38d354fa |
|
The failures in the extended builds are not connected to the changes in this PR. Thanks again @ayushbansal07! |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 37c87db. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 12 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…ptions (apache#47610) ### Rationale for this change Expose CSV writer option quoting_header for pyarrow. Addresses apache#47575 ### What changes are included in this PR? Cython changes for parsing quoting_header option in a manner similar to quoting_style ### Are these changes tested? Yes, added a unit test under test_csv.py ### Are there any user-facing changes? Add QuotingStyle quoting_header option in WriteOptions for pyarrow * GitHub Issue: apache#47575 Authored-by: Ayush Bansal <[email protected]> Signed-off-by: AlenkaF <[email protected]>
Rationale for this change
Expose CSV writer option quoting_header for pyarrow. Addresses #47575
What changes are included in this PR?
Cython changes for parsing quoting_header option in a manner similar to quoting_style
Are these changes tested?
Yes, added a unit test under test_csv.py
Are there any user-facing changes?
Add QuotingStyle quoting_header option in WriteOptions for pyarrow
quoting_header#47575