Skip to content

GH-39779: [Python] Expose force_virtual_addressing in PyArrow#39819

Merged
jorisvandenbossche merged 3 commits intoapache:mainfrom
xpgcrx:expose-s3-fva
Feb 1, 2024
Merged

GH-39779: [Python] Expose force_virtual_addressing in PyArrow#39819
jorisvandenbossche merged 3 commits intoapache:mainfrom
xpgcrx:expose-s3-fva

Conversation

@xpgcrx
Copy link
Contributor

@xpgcrx xpgcrx commented Jan 27, 2024 •
edited
Loading

Rationale for this change / What changes are included in this PR?

To expose force_virtual_addressing in PyArrow.

Are these changes tested?

Existing unit tests are not broken, and a new test case have been added.

Are there any user-facing changes?

pyarrow.fs.S3FileSystem: it becomes possible to specify the argument 'force_virtual_addressing'.

@github-actions
Copy link

github-actions bot commented Jan 27, 2024

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

@AlenkaF
Copy link
Member

AlenkaF commented Jan 29, 2024

Thank you for you contribution @yo1956 !

From a quick look I think that the Cython layer will have to be updated to include the force_virtual_addressing

arrow/python/pyarrow/includes/libarrow_fs.pxd

Lines 161 to 185 in 21ffd82

cdef cppclass CS3Options "arrow::fs::S3Options":
c_string region
double connect_timeout
double request_timeout
c_string endpoint_override
c_string scheme
c_bool background_writes
c_bool allow_bucket_creation
c_bool allow_bucket_deletion
shared_ptr[const CKeyValueMetadata] default_metadata
c_string role_arn
c_string session_name
c_string external_id
int load_frequency
CS3ProxyOptions proxy_options
CS3CredentialsKind credentials_kind
shared_ptr[CS3RetryStrategy] retry_strategy
void ConfigureDefaultCredentials()
void ConfigureAccessKey(const c_string& access_key,
const c_string& secret_key,
const c_string& session_token)
c_string GetAccessKey()
c_string GetSecretKey()
c_string GetSessionToken()
c_bool Equals(const CS3Options& other)

@xpgcrx
Copy link
Contributor Author

xpgcrx commented Jan 29, 2024

@AlenkaF
Thank you for reviewing!
I'll fix it.

@xpgcrx
Copy link
Contributor Author

xpgcrx commented Jan 29, 2024

@AlenkaF
I've made the modifications, but it seems there are errors with CI that doesn't appear to be related to my changes.
I'm not entirely sure, though. I would be grateful if you could provide some advice when you have time.

@AlenkaF
Copy link
Member

AlenkaF commented Jan 29, 2024

The failures are not related, there is a fix already open: #39827. Similar for the Sphinx build.

We can wait for them to get merged and rebase then.

AlenkaF
AlenkaF approved these changes Jan 30, 2024
View reviewed changes
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.

LGTM, thank you for the updates!

I do not think there are any additions needed in the tests, but would like to have one extra approve before merging.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 30, 2024
@xpgcrx
Copy link
Contributor Author

xpgcrx commented Jan 30, 2024

LGTM, thank you for the updates!

I do not think there are any additions needed in the tests, but would like to have one extra approve before merging.

@AlenkaF
Thank you!!

@xpgcrx
Copy link
Contributor Author

xpgcrx commented Feb 1, 2024

@jorisvandenbossche
Could you please review this when you have time?

jorisvandenbossche
jorisvandenbossche approved these changes Feb 1, 2024
View reviewed changes
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.

Looks good, thanks!

@jorisvandenbossche jorisvandenbossche merged commit 3d45ac9 into apache:main Feb 1, 2024
@jorisvandenbossche jorisvandenbossche removed the awaiting committer review Awaiting committer review label Feb 1, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Feb 1, 2024
@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Feb 2, 2024

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 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
@xpgcrx @dgreiss
apacheGH-39779: [Python] Expose force_virtual_addressing in PyArrow (a…
4e2e49c 
…pache#39819)

### Rationale for this change / What changes are included in this PR?

To expose force_virtual_addressing in PyArrow.

### Are these changes tested?

Existing unit tests are not broken, and a new test case have been added.

### Are there any user-facing changes?

pyarrow.fs.S3FileSystem: it becomes possible to specify the argument 'force_virtual_addressing'.

* Closes: apache#39779

Authored-by: yo1956 <[email protected]>
Signed-off-by: Joris Van den Bossche <[email protected]>
@kadisi kadisi mentioned this pull request Mar 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

@AlenkaF AlenkaF AlenkaF approved these changes

Assignees

No one assigned

Labels

awaiting merge Awaiting merge Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Python] Expose force_virtual_addressing in PyArrow

3 participants

@xpgcrx @AlenkaF @jorisvandenbossche