This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-4005-4dba4a480bfe7548bcb99e1c4194f692d2975d00 in repository https://gitbox.apache.org/repos/asf/iceberg-python.git
commit d888dd113efc5bddd650ccdd17c64f8866ccf07b Author: Rodrigo Stachlewski Palma <[email protected]> AuthorDate: Thu Sep 24 19:44:47 2026 +0000 fix(config): let an explicitly falsy value on the right-hand side win in merge_config (#4005) fix(config): let an explicitly falsy value on the right-hand side win merge_config() documents 'take the non-null value, with precedence on rhs', but it used `rhs_value or lhs_value`, which falls back to the left-hand side for any falsy value, not just None. load_catalog(name, **properties) merges the configuration file into the caller's properties, so turning an option off explicitly was silently ignored when the file set it: # .pyiceberg.yaml has s3.path-style-access: "true" load_catalog("prod", **{"s3.path-style-access": False}) # -> 's3.path-style-access' stays 'true' That contradicts the intent of #45, which made the passed-in argument take precedence over the environment. property_as_bool() in the same codebase already distinguishes 'not set' from falsy by testing against (None, ""), so the two now agree. None on the right-hand side still means 'not set' and keeps the left-hand value. --- pyiceberg/utils/config.py | 5 +++-- tests/utils/test_config.py | 24 +++++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/pyiceberg/utils/config.py b/pyiceberg/utils/config.py index 2b5baafa5..4d58365d5 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 309821023..17b495f92 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"] [email protected]("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:
