Fix fsspec S3 virtual addressing boolean parsing#3492
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 dev@iceberg.apache.org 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)
3 participants
Rationale for this change
FsspecFileIOcheckeds3.force-virtual-addressingwith raw property truthiness. That meant"false"still enabled virtual-host addressing because it is a non-empty string.This changes the fsspec S3 setup to use the existing boolean property parser, matching the rest of the codebase and preserving the default behavior when the property is absent.
Are these changes tested?
PYTHONPATH=. uv run --extra s3fs pytest tests/io/test_fsspec.py::test_fsspec_s3_session_properties_force_virtual_addressingPYTHONPATH=. uv run --extra s3fs pytest tests/io/test_fsspec.py -k "s3_session_properties"uv run prek run ruff --files pyiceberg/io/fsspec.py tests/io/test_fsspec.pyuv run prek run ruff-format --files pyiceberg/io/fsspec.py tests/io/test_fsspec.pyAre there any user-facing changes?
Yes. Setting
s3.force-virtual-addressing=falsefor fsspec S3 FileIO now leaves the default addressing behavior unchanged instead of forcing virtual addressing.