diff --git a/pyiceberg/io/fsspec.py b/pyiceberg/io/fsspec.py index 7749268ff5..ec26255f5a 100644 --- a/pyiceberg/io/fsspec.py +++ b/pyiceberg/io/fsspec.py @@ -201,7 +201,7 @@ def _s3(properties: Properties) -> AbstractFileSystem: 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): + if property_as_bool(properties, S3_FORCE_VIRTUAL_ADDRESSING, False): config_kwargs["s3"] = {"addressing_style": "virtual"} if s3_anonymous := properties.get(S3_ANONYMOUS): diff --git a/pyiceberg/utils/properties.py b/pyiceberg/utils/properties.py index 2a95b39a50..2aa8728ac4 100644 --- a/pyiceberg/utils/properties.py +++ b/pyiceberg/utils/properties.py @@ -54,13 +54,13 @@ def property_as_float( def property_as_bool( - properties: dict[str, str], + properties: Properties, property_name: str, default: bool, ) -> bool: - if value := properties.get(property_name): + if (value := properties.get(property_name)) not in (None, ""): try: - return strtobool(value) + return strtobool(str(value)) except ValueError as e: raise ValueError(f"Could not parse table property {property_name} to a boolean: {value}") from e return default diff --git a/tests/io/test_fsspec.py b/tests/io/test_fsspec.py index 8739a5964d..cc59ae3998 100644 --- a/tests/io/test_fsspec.py +++ b/tests/io/test_fsspec.py @@ -320,9 +320,20 @@ def test_fsspec_s3_session_properties() -> None: ) -def test_fsspec_s3_session_properties_force_virtual_addressing() -> None: +@pytest.mark.parametrize( + ("force_virtual_addressing", "config_kwargs"), + [ + ("true", {"s3": {"addressing_style": "virtual"}}), + ("false", {}), + (True, {"s3": {"addressing_style": "virtual"}}), + (False, {}), + ], +) +def test_fsspec_s3_session_properties_force_virtual_addressing( + force_virtual_addressing: str | bool, config_kwargs: Properties +) -> None: session_properties: Properties = { - "s3.force-virtual-addressing": True, + "s3.force-virtual-addressing": force_virtual_addressing, "s3.endpoint": "http://localhost:9000", "s3.access-key-id": "admin", "s3.secret-access-key": "password", @@ -346,7 +357,7 @@ def test_fsspec_s3_session_properties_force_virtual_addressing() -> None: "region_name": "us-east-1", "aws_session_token": "s3.session-token", }, - config_kwargs={"s3": {"addressing_style": "virtual"}}, + config_kwargs=config_kwargs, ) diff --git a/tests/utils/test_properties.py b/tests/utils/test_properties.py index 2cb4ea5ace..9e02cd5e69 100644 --- a/tests/utils/test_properties.py +++ b/tests/utils/test_properties.py @@ -67,14 +67,23 @@ def test_property_as_float_with_invalid_value() -> None: assert "Could not parse table property some_float_prop to a float: invalid" in str(exc.value) -def test_property_as_bool() -> None: +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("True", True), + ("False", False), + (True, True), + (False, False), + ], +) +def test_property_as_bool(value: str | bool, expected: bool) -> None: properties = { - "bool": "True", + "bool": value, } - assert property_as_bool(properties, "bool", default=False) is True + assert property_as_bool(properties, "bool", default=not expected) is expected assert property_as_bool(properties, "missing", default=False) is False - assert property_as_float(properties, "missing") is None + assert property_as_bool(properties, "missing", default=True) is True def test_property_as_bool_with_invalid_value() -> None: