diff --git a/pyiceberg/utils/config.py b/pyiceberg/utils/config.py index 2b5baafa59..4d58365d52 100644 --- a/pyiceberg/utils/config.py +++ b/pyiceberg/utils/config.py @@ -42,8 +42,9 @@ def merge_config(lhs: RecursiveDict, rhs: RecursiveDict) -> RecursiveDict: # If they are both dicts, then we have to go deeper new_config[rhs_key] = merge_config(lhs_value, rhs_value) else: - # Take the non-null value, with precedence on rhs - new_config[rhs_key] = rhs_value or lhs_value + # Take the non-null value, with precedence on rhs. `None` means "not set", + # while a falsy value such as `False` or `0` is an explicit setting and wins. + new_config[rhs_key] = rhs_value if rhs_value is not None else lhs_value else: # New key new_config[rhs_key] = rhs_value diff --git a/tests/utils/test_config.py b/tests/utils/test_config.py index 309821023d..17b495f929 100644 --- a/tests/utils/test_config.py +++ b/tests/utils/test_config.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. import os -from typing import Any +from typing import Any, cast from unittest import mock import pytest @@ -87,6 +87,28 @@ def test_merge_config() -> None: assert result["common_key"] == rhs["common_key"] +@pytest.mark.parametrize("falsy_value", [False, "", 0]) +def test_merge_config_rhs_wins_for_falsy_values(falsy_value: Any) -> None: + """A value set explicitly on the right-hand side wins even when it is falsy. + + `load_catalog(name, **properties)` merges the configuration file into the properties + passed by the caller, so turning an option off explicitly must not fall back to the + value coming from the file. + """ + lhs: RecursiveDict = {"s3.path-style-access": "true"} + rhs: RecursiveDict = {"s3.path-style-access": falsy_value} + result = merge_config(lhs, rhs) + assert result["s3.path-style-access"] == falsy_value + + +def test_merge_config_lhs_wins_when_rhs_is_none() -> None: + """`None` on the right-hand side means "not set", so the left-hand side survives.""" + lhs: RecursiveDict = {"uri": "https://example.com"} + rhs = cast(RecursiveDict, {"uri": None}) + result = merge_config(lhs, rhs) + assert result["uri"] == "https://example.com" + + def test_from_configuration_files_get_typed_value(tmp_path_factory: pytest.TempPathFactory) -> None: config_path = str(tmp_path_factory.mktemp("config")) with open(f"{config_path}/.pyiceberg.yaml", "w", encoding=UTF8) as file: