Rodrigo-Palma opened a new pull request, #4005: URL: https://github.com/apache/iceberg-python/pull/4005
# Rationale `merge_config()` says it takes "the non-null value, with precedence on rhs", but the implementation is: ```python new_config[rhs_key] = rhs_value or lhs_value ``` `or` falls back to the left-hand side for *any* falsy value, not just `None`. 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. ```python # ~/.pyiceberg.yaml # catalog: # prod: # type: rest # uri: https://example.com # s3.path-style-access: "true" from pyiceberg.catalog import load_catalog catalog = load_catalog("prod", **{"s3.path-style-access": False}) # s3.path-style-access is still "true" ``` The same happens for `""` and `0`: | rhs value | documented result | actual result | |---|---|---| | `False` | `False` | `"true"` (from the file) | | `""` | `""` | `"true"` | | `0` | `0` | `"true"` | | `None` | value from the file | value from the file (correct) | This 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 `False` is passing an argument. There is also a precedent inside this codebase: `property_as_bool()` in `pyiceberg/utils/properties.py` already distinguishes "not set" from "falsy", with `if (value := properties.get(property_name)) not in (None, "")`. After this change the two agree instead of disagreeing. # Change ```python new_config[rhs_key] = rhs_value if rhs_value is not None else lhs_value ``` `None` still means "not set" and still lets the left-hand value survive, which is what the `_from_environment_variables` path relies on. Tests added to `tests/utils/test_config.py`: - `test_merge_config_rhs_wins_for_falsy_values`, parametrized over `False`, `""` and `0`. - `test_merge_config_lhs_wins_when_rhs_is_none`, pinning the `None` behavior so a future change cannot quietly turn `None` into an override. # Verification - `make lint` passes (ruff, ruff format, mypy, pydocstyle, codespell). - `pytest tests/utils tests/catalog tests/cli` passes: 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 in `tests/avro/test_decoder.py` for `CythonBinaryDecoder` and 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
