Skip to content

Conversation

@HaochengLIU
Copy link
Contributor

@HaochengLIU HaochengLIU commented Jun 5, 2024

Rationale for this change

Expose new S3 option check_directory_existence_before_creation from GH-41493

What changes are included in this PR?

Expose new S3 option check_directory_existence_before_creation from GH-41493

Are these changes tested?

yes

Are there any user-facing changes?

Yes. Python function documentation is updated.

@github-actions
Copy link

github-actions bot commented Jun 5, 2024

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

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 5, 2024
@HaochengLIU
Copy link
Contributor Author

@pitrou Here is the Python side exposure PR
I have very limited knowledge about CPython as I normally use Pybind11, I just follow the existing code for allow_bucket_deletion
Please review when you have cycle 😃
ty

@pitrou pitrou requested a review from jorisvandenbossche June 5, 2024 06:42
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

The code looks good! Just some comments on the docstring

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
check_directory_existence_before_creation: boolean, default false
check_directory_existence_before_creation : boolean, default false

Copy link
Member

Choose a reason for hiding this comment

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

It's rather unclear what "pessimistic" means in this context (but I see this term is used on the C++ side as well). Also, we shouldn't mention "CreateDir" as that is a C++ function the python user is not familiar with. We can use a generic term, something like "when creating a directory"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if false, then CreateDir will try to create the directory without checking its
By default (False), when creating a directory, it will try to create the directory without checking its

I would maybe also include the "It's an optimization to try directory creation and catch the error, rather than issue two dependent I/O calls." as is explained in C++

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 5, 2024
@HaochengLIU HaochengLIU force-pushed the check_directory_existence_before_creation-expose-python branch from 87d9d37 to 56f6875 Compare June 6, 2024 00:03
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 6, 2024
@HaochengLIU
Copy link
Contributor Author

@jorisvandenbossche thanks for the suggestions, please review again

@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review awaiting merge Awaiting merge and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Jun 6, 2024
@jorisvandenbossche jorisvandenbossche merged commit b51e997 into apache:main Jun 7, 2024
@jorisvandenbossche jorisvandenbossche removed the awaiting merge Awaiting merge label Jun 7, 2024
@kou kou changed the title GH-41960: Expose new S3 option check_directory_existence_before_creation GH-41960: [Python] Expose new S3 option check_directory_existence_before_creation Jun 8, 2024
@github-actions
Copy link

github-actions bot commented Jun 8, 2024

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

@conbench-apache-arrow
Copy link

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

There were 8 benchmark results indicating a performance regression:

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

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.

3 participants