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

GH-38857: [Python] Add append mode for pyarrow.OsFile #38820

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

milesgranger
Copy link
Contributor

@milesgranger milesgranger commented Nov 21, 2023

Rationale for this change

Seems reasonable. :)

What changes are included in this PR?

Exposes the C++ append parameter from FileOutputStream to PyArrow's OSFile.

Are these changes tested?

Yes.

Are there any user-facing changes?

Can add a or ab to mode parameter in pyarrow.OsFile

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @milesgranger !
This PR doesn't seem to fit our definition for MINOR, as it should be only about minor documentation changes.
Could you please create a GitHub issue and update the PR title accordingly?
Thanks!

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Nov 23, 2023
@milesgranger milesgranger changed the title MINOR: [Python] Add append mode for pyarrow.OsFile GH-38857: [Python] Add append mode for pyarrow.OsFile Nov 23, 2023
Copy link

⚠️ GitHub issue #38857 has been automatically assigned in GitHub to PR creator.

@milesgranger
Copy link
Contributor Author

Ah, how embarrassing. Issue created and and title updated as requested!

@raulcd raulcd added awaiting review Awaiting review and removed awaiting changes Awaiting changes labels Nov 23, 2023
Copy link
Member

@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

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

Hi @milesgranger, thank you for the PR!
LGTM +1

Would only add an example to the docstrings of the OSFile class. It would maybe also be worth adding a test to test_native_file_modes?

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 27, 2023
@milesgranger
Copy link
Contributor Author

milesgranger commented Nov 28, 2023

Thanks for the review @AlenkaF.
In adding the doc example and looking at test_native_file_modes, I see that NativeFile.mode plays only with 'is_readable' and 'is_writable'.

Should I go about adding a is_appendable to facilitate returning an 'ab'?


Edit: happy to do a follow up as well. Whichever is easier for you all.

@AlenkaF
Copy link
Member

AlenkaF commented Nov 28, 2023

Ah, thank you for looking into it.

I am not sure how to handle 'ab' mode in connection to the NativeFile class. I do not see an issue if OSFile subclass handles extra append mode and NativeFile does not. @jorisvandenbossche what do you think?

@pitrou
Copy link
Member

pitrou commented Nov 29, 2023

Should I go about adding a is_appendable to facilitate returning an 'ab'?

You could add a private _is_appending attribute that would default to False.

@milesgranger
Copy link
Contributor Author

Failure seems unrelated.

@AlenkaF
Copy link
Member

AlenkaF commented Nov 30, 2023

I have rerun it just in case.
LGTM otherwise, thank you!

@raulcd
Copy link
Member

raulcd commented Nov 30, 2023

I also think is unrelated. I've opened #39003 to track it.

@AlenkaF AlenkaF merged commit 92fe831 into apache:main Nov 30, 2023
11 of 12 checks passed
@AlenkaF AlenkaF removed the awaiting committer review Awaiting committer review label Nov 30, 2023
@AlenkaF
Copy link
Member

AlenkaF commented Nov 30, 2023

Thanks! 👋

@milesgranger milesgranger deleted the osfile-append branch November 30, 2023 18:40
@jorisvandenbossche
Copy link
Member

Thanks Miles! ;)

jorisvandenbossche added a commit that referenced this pull request Dec 1, 2023
### Rationale for this change

Small fixup of the change in #38820 to fix the build failure on cython 2 (nightly crossbow build)
* Closes: #38857

Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 92fe831.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…8820)

### Rationale for this change

Seems reasonable. :) 

### What changes are included in this PR?

Exposes the C++ append parameter from FileOutputStream  to PyArrow's OSFile.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Can add `a` or `ab` to `mode` parameter in `pyarrow.OsFile`

* Closes: apache#38857

Authored-by: Miles Granger <miles59923@gmail.com>
Signed-off-by: AlenkaF <frim.alenka@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
### Rationale for this change

Small fixup of the change in apache#38820 to fix the build failure on cython 2 (nightly crossbow build)
* Closes: apache#38857

Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Python] Expose append parameter from FileOutputStream::Open
5 participants