fix(config): let an explicitly falsy value on the right-hand side win in merge_config - #4005
Open
Rodrigo-Palma wants to merge 1 commit into
Open
Rodrigo-Palma wants to merge 1 commit into
Rodrigo-Palma wants to merge 1 commit into
Conversation
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 apache#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
merge_config()says it takes "the non-null value, with precedence on rhs", but the implementation is:orfalls back to the left-hand side for any falsy value, not justNone. The right-hand side is the caller's own properties, so explicitly turning an option off is silently discarded when the configuration file sets it.The same happens for
""and0:FalseFalse"true"(from the file)"""""true"00"true"NoneThis contradicts what #45 set out to do. That PR swapped the merge order precisely so that "the passed in argument takes precedence" over configuration coming from the environment, and a caller passing
Falseis passing an argument.There is also a precedent inside this codebase:
property_as_bool()inpyiceberg/utils/properties.pyalready distinguishes "not set" from "falsy", withif (value := properties.get(property_name)) not in (None, ""). After this change the two agree instead of disagreeing.Change
Nonestill means "not set" and still lets the left-hand value survive, which is what the_from_environment_variablespath relies on.Tests added to
tests/utils/test_config.py:test_merge_config_rhs_wins_for_falsy_values, parametrized overFalse,""and0.test_merge_config_lhs_wins_when_rhs_is_none, pinning theNonebehavior so a future change cannot quietly turnNoneinto an override.Verification
make lintpasses (ruff, ruff format, mypy, pydocstyle, codespell).pytest tests/utils tests/catalog tests/clipasses: 1434 tests.pytest tests/ -m "not integration and not s3 and not adls and not gcs" --ignore=tests/io --ignore=tests/benchmark: 3900 passed, 9 failed. The 9 failures are all intests/avro/test_decoder.pyforCythonBinaryDecoderand are pre-existing in my environment: the same 9 fail on the unmodified branch at the same commit (9 failed, 59 passed both with and without this patch), since the Cython extension is not built locally.