Skip to content

GH-49078: [FS][Azure] Fix lossy pickling of SubTreeFileSystem(base_path, AzureFileSystem(...))#49140

Merged
raulcd merged 18 commits intoapache:mainfrom
Tom-Newton:tomnewton/fix_lossy_pickling_of_azure_filesystem/GH-49078
Mar 2, 2026
Merged

GH-49078: [FS][Azure] Fix lossy pickling of SubTreeFileSystem(base_path, AzureFileSystem(...))#49140
raulcd merged 18 commits intoapache:mainfrom
Tom-Newton:tomnewton/fix_lossy_pickling_of_azure_filesystem/GH-49078

Conversation

@Tom-Newton
Copy link
Contributor

@Tom-Newton Tom-Newton commented Feb 4, 2026

Rationale for this change

Fix #49078

What changes are included in this PR?

  • Implement getters on the C++ side of AzureOptions, for the values that are currently stored only on the python side.
    • This required adding some more member variables
    • I decided to add ClearCredentials , so that it can't get into strange states by configuring one credential type then another. IMO configuring the credentials during initialisation on the AzureOptions would be neater but I don't want to make this PR too big.
  • Update the C++ side AzureOptions::Equals
  • Remove python side self attributes and instead depend on getters from C++ side.

Are these changes tested?

  • Updated tests on the C++ side for the updated Equals and newly added getter methods.
  • Added a fixture pickle_with_and_without_subtree_filesystem, which can be used in place of the pickle_module. This adds test combinations with and without wrapping the filesystem in a SubTreeFilesystem before pickling it.

Are there any user-facing changes?

Only that pickling SubTreeFileSystem(base_path, AzureFileSystem(...)) now works properly.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026

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

@Tom-Newton Tom-Newton force-pushed the tomnewton/fix_lossy_pickling_of_azure_filesystem/GH-49078 branch from 28ad544 to d0a02e3 Compare February 6, 2026 00:25
@Tom-Newton Tom-Newton marked this pull request as ready for review February 6, 2026 09:01
@Tom-Newton
Copy link
Contributor Author

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?

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.

Hi @Tom-Newton thanks for being patient. There are lots of PRs and issues being opened and sometimes it just takes a while to find the time. Just a really minor nit but the changes look reasonable to me.

@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Feb 25, 2026
@Tom-Newton Tom-Newton force-pushed the tomnewton/fix_lossy_pickling_of_azure_filesystem/GH-49078 branch from 012f6c5 to 648800f Compare February 27, 2026 17:40
@Tom-Newton Tom-Newton force-pushed the tomnewton/fix_lossy_pickling_of_azure_filesystem/GH-49078 branch from 648800f to 9499abc Compare February 27, 2026 17:41
@Tom-Newton
Copy link
Contributor Author

Rebasing on main in the hope it will fix a bunch of test failures that didn't look related.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 2, 2026
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 2, 2026
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 @Tom-Newton

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Mar 2, 2026
@raulcd raulcd merged commit 0124d5b into apache:main Mar 2, 2026
52 of 56 checks passed
@raulcd raulcd removed the awaiting merge Awaiting merge label Mar 2, 2026
@Tom-Newton
Copy link
Contributor Author

Thanks for the PR @Tom-Newton

Thanks for reviewing 🙂

@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 44 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.

[Python][FS][Azure] Pickling SubTreeFileSystem(base_path, AzureFileSystem(...)) is lossy

2 participants