You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Tom-Newton (via GitHub)" <gi...@apache.org> on 2026/02/04 11:18:35 UTC
[PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Tom-Newton opened a new pull request, #49140:
URL: https://github.com/apache/arrow/pull/49140
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
Please remove this line and the above text before creating your pull request.
### Rationale for this change
Fix https://github.com/apache/arrow/issues/49078
### What changes are included in this PR?
Don't store any paramaters on the python side object.
### Are these changes tested?
### Are there any user-facing changes?
**This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
**This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on PR #49140:
URL: https://github.com/apache/arrow/pull/49140#issuecomment-3913106750
Sorry to be impatient, but please could someone review this? The test failure looks like a timeout, that I don't think is related. @AlenkaF please could you review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #49140:
URL: https://github.com/apache/arrow/pull/49140#discussion_r2871406088
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -247,13 +247,17 @@ Result<AzureOptions> AzureOptions::FromUri(const std::string& uri_string,
}
bool AzureOptions::Equals(const AzureOptions& other) const {
- const bool equals = blob_storage_authority == other.blob_storage_authority &&
- dfs_storage_authority == other.dfs_storage_authority &&
- blob_storage_scheme == other.blob_storage_scheme &&
- dfs_storage_scheme == other.dfs_storage_scheme &&
- default_metadata == other.default_metadata &&
- account_name == other.account_name &&
- credential_kind_ == other.credential_kind_;
+ const bool equals =
+ account_name == other.account_name &&
+ blob_storage_authority == other.blob_storage_authority &&
+ dfs_storage_authority == other.dfs_storage_authority &&
+ blob_storage_scheme == other.blob_storage_scheme &&
+ dfs_storage_scheme == other.dfs_storage_scheme &&
+ default_metadata == other.default_metadata &&
+ background_writes == other.background_writes &&
+ credential_kind_ == other.credential_kind_ && account_key_ == other.account_key_ &&
Review Comment:
I should have payed more attention to that myself. I think its happy again now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #49140:
URL: https://github.com/apache/arrow/pull/49140#discussion_r2871306424
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -247,13 +247,17 @@ Result<AzureOptions> AzureOptions::FromUri(const std::string& uri_string,
}
bool AzureOptions::Equals(const AzureOptions& other) const {
- const bool equals = blob_storage_authority == other.blob_storage_authority &&
- dfs_storage_authority == other.dfs_storage_authority &&
- blob_storage_scheme == other.blob_storage_scheme &&
- dfs_storage_scheme == other.dfs_storage_scheme &&
- default_metadata == other.default_metadata &&
- account_name == other.account_name &&
- credential_kind_ == other.credential_kind_;
+ const bool equals =
+ account_name == other.account_name &&
+ blob_storage_authority == other.blob_storage_authority &&
+ dfs_storage_authority == other.dfs_storage_authority &&
+ blob_storage_scheme == other.blob_storage_scheme &&
+ dfs_storage_scheme == other.dfs_storage_scheme &&
+ default_metadata == other.default_metadata &&
+ background_writes == other.background_writes &&
+ credential_kind_ == other.credential_kind_ && account_key_ == other.account_key_ &&
Review Comment:
oops, it seems the linter really dislikes this:
```
pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index 5820316f20..20b0a655d8 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc
+++ b/cpp/src/arrow/filesystem/azurefs.cc
@@ -256,12 +256,9 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
dfs_storage_scheme == other.dfs_storage_scheme &&
default_metadata == other.default_metadata &&
background_writes == other.background_writes &&
- credential_kind_ == other.credential_kind_ &&
- account_key_ == other.account_key_ &&
- sas_token_ == other.sas_token_ &&
- tenant_id_ == other.tenant_id_ &&
- client_id_ == other.client_id_ &&
- client_secret_ == other.client_secret_;
+ credential_kind_ == other.credential_kind_ && account_key_ == other.account_key_ &&
+ sas_token_ == other.sas_token_ && tenant_id_ == other.tenant_id_ &&
+ client_id_ == other.client_id_ && client_secret_ == other.client_secret_;
if (!equals) {
```
I am sorry about that :( btw, the rest of CI failures are unrelated. There's a known issue with the aws-sdk on homebrew that should be solved during the next days when a new version is available there.
I am happy to merge once the linter is fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on PR #49140:
URL: https://github.com/apache/arrow/pull/49140#issuecomment-3983594795
> Thanks for the PR @Tom-Newton
Thanks for reviewing 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49140:
URL: https://github.com/apache/arrow/pull/49140#issuecomment-3846851076
:warning: GitHub issue #49078 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49140:
URL: https://github.com/apache/arrow/pull/49140#issuecomment-3988153198
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0124d5b5b7f60b6ae6312bbb962dcff6dd4bc641.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/65494991759) has more details. It also includes information about 44 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #49140:
URL: https://github.com/apache/arrow/pull/49140#discussion_r2851631529
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -247,13 +247,17 @@ Result<AzureOptions> AzureOptions::FromUri(const std::string& uri_string,
}
bool AzureOptions::Equals(const AzureOptions& other) const {
- const bool equals = blob_storage_authority == other.blob_storage_authority &&
- dfs_storage_authority == other.dfs_storage_authority &&
- blob_storage_scheme == other.blob_storage_scheme &&
- dfs_storage_scheme == other.dfs_storage_scheme &&
- default_metadata == other.default_metadata &&
- account_name == other.account_name &&
- credential_kind_ == other.credential_kind_;
+ const bool equals =
+ account_name == other.account_name &&
+ blob_storage_authority == other.blob_storage_authority &&
+ dfs_storage_authority == other.dfs_storage_authority &&
+ blob_storage_scheme == other.blob_storage_scheme &&
+ dfs_storage_scheme == other.dfs_storage_scheme &&
+ default_metadata == other.default_metadata &&
+ background_writes == other.background_writes &&
+ credential_kind_ == other.credential_kind_ && account_key_ == other.account_key_ &&
Review Comment:
Can you keep one comparison per line and using that you have refactored it a bit keep them in alphabetical order for easier review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49078: [FS][Azure] Fix lossy pickling of `SubTreeFileSystem(base_path, AzureFileSystem(...))` [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd merged PR #49140:
URL: https://github.com/apache/arrow/pull/49140
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org