fix(fsspec): parse S3 virtual addressing as a boolean#3492
fix(fsspec): parse S3 virtual addressing as a boolean#3492fallintoplace wants to merge 1 commit into
Conversation
ebyhr
left a comment
ebyhr
left a comment
There was a problem hiding this comment.
Looks good to me. I've confirmed that the added test fails without this PR's change.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
Let me know if anything needs fixing. |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
This look great. Nice simple bug fix. Thanks for doing this!
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Great bug fix. Thanks for doing this!
| if request_timeout := properties.get(S3_REQUEST_TIMEOUT): | ||
| config_kwargs["read_timeout"] = float(request_timeout) | ||
|
|
||
| if _force_virtual_addressing := properties.get(S3_FORCE_VIRTUAL_ADDRESSING): |
There was a problem hiding this comment.
There's probably more of these issues throughout the codebase. Ideally, we could make a full Properties class that could handle most of this for us (instead of properties just being a dict)
2e4cdc3 to
9d33da7
Compare
fallintoplace
changed the title
3 participants
Rationale for this change
FsspecFileIOevaluateds3.force-virtual-addressingusing the raw truthiness of the property value. As a result, the non-empty string"false"incorrectly enabled virtual-hosted-style S3 addressing.This change uses the shared
property_as_boolutility so string and native boolean values are parsed consistently. The utility now accepts the repository'sPropertiestype and preserves support for nativeboolvalues. When the property is absent, fsspec retains its existing default addressing behavior.A dedicated properties abstraction could prevent similar mistakes more broadly, but that larger refactor is outside this targeted fix.
Are these changes tested?
Yes. The fsspec S3 session-property test now covers
"true","false",True, andFalse. The shared property utility tests cover the same input forms and verify default handling.The following checks pass locally:
PYTHONPATH=. uv run --extra s3fs pytest tests/io/test_fsspec.py -k "s3_session_properties" -qPYTHONPATH=. uv run pytest tests/utils/test_properties.py -quv run prek run --files pyiceberg/io/fsspec.py pyiceberg/utils/properties.py tests/io/test_fsspec.py tests/utils/test_properties.pyAre there any user-facing changes?
Yes. Setting
s3.force-virtual-addressing=falsefor fsspec S3 FileIO no longer enables virtual-hosted-style addressing. String and native boolean values now produce consistent results, while an absent property continues to use the existing default.