kevinjqliu commented on code in PR #3441: URL: https://github.com/apache/iceberg-python/pull/3441#discussion_r3447742809
########## tests/test_environment_context.py: ########## @@ -0,0 +1,34 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import re + +from pyiceberg.environment_context import EnvironmentContext + + +def test_default_value() -> None: + actual = EnvironmentContext.get() + assert len(actual) == 2 + assert actual["engine-name"] == "pyiceberg" + assert re.match(r"^\d+\.\d+\.\d+", actual["engine-version"]) Review Comment: ```suggestion def test_default_value() -> None: assert EnvironmentContext.get() == { "engine-name": "pyiceberg", "engine-version": __version__, } def test_get_returns_copy() -> None: actual = EnvironmentContext.get() actual["test-key"] = "test-value" assert "test-key" not in EnvironmentContext.get() ``` EnvironmentContext might have been mutated, this is a better way to test that the returned value is a copy ########## pyiceberg/view/metadata.py: ########## @@ -51,7 +52,7 @@ class ViewVersion(IcebergBaseModel): """ID of the schema for the view version""" timestamp_ms: int = Field(alias="timestamp-ms", default_factory=lambda: int(time.time() * 1000)) """Timestamp when the version was created (ms from epoch)""" - summary: dict[str, str] = Field(default_factory=dict) + summary: dict[str, str] = Field(default_factory=lambda: EnvironmentContext.get()) Review Comment: this will add `EnvironmentContext` to all `ViewVersion` which might not be the desired behavior. Similar to Table Metadata, can we only add EnvironmentContext in the write path? ########## pyiceberg/environment_context.py: ########## @@ -0,0 +1,43 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from importlib.metadata import version + + +class EnvironmentContext: + _PROPERTIES: dict[str, str] = { + "engine-name": "pyiceberg", + "engine-version": version("pyiceberg"), Review Comment: ```suggestion "engine-version": __version__, ``` nit ########## tests/integration/test_writes/test_partitioned_writes.py: ########## @@ -498,6 +499,8 @@ def test_summaries_with_null(spark: SparkSession, session_catalog: Catalog, arro "total-files-size": str(file_size), "total-position-deletes": "0", "total-records": "3", + "engine-name": "pyiceberg", Review Comment: Could we find a way to avoid repeating env context everywhere? Perhaps a small test helper like this would keep the exact assertions while localizing this new default behavior: ``` def with_environment_context(summary: dict[str, str]) -> dict[str, str]: return {**summary, **EnvironmentContext.get()} assert summaries[0] == with_environment_context({ "added-data-files": "3", "added-records": "5", ... }) ``` ########## pyiceberg/table/snapshots.py: ########## @@ -402,6 +403,9 @@ def _update_totals(total_property: str, added_property: str, removed_property: s removed_property=REMOVED_EQUALITY_DELETES, ) + for key, value in EnvironmentContext.get().items(): + summary.__setitem__(key, value) Review Comment: ```suggestion for key, value in EnvironmentContext.get().items(): summary[key] = value ``` nit ########## tests/test_environment_context.py: ########## @@ -0,0 +1,34 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import re + +from pyiceberg.environment_context import EnvironmentContext + + +def test_default_value() -> None: + actual = EnvironmentContext.get() + assert len(actual) == 2 + assert actual["engine-name"] == "pyiceberg" + assert re.match(r"^\d+\.\d+\.\d+", actual["engine-version"]) + + +def test_put_and_remove() -> None: + EnvironmentContext.put("test-key", "test-value") + assert EnvironmentContext.get()["test-key"] == "test-value" + + EnvironmentContext.remove("test-key") + assert "test-key" not in EnvironmentContext.get() Review Comment: ```suggestion def test_put_and_remove() -> None: try: EnvironmentContext.put("test-key", "test-value") assert EnvironmentContext.get()["test-key"] == "test-value" assert EnvironmentContext.remove("test-key") == "test-value" assert "test-key" not in EnvironmentContext.get() finally: EnvironmentContext.remove("test-key") ``` use try/finally to avoid mutating EnvironmentContext -- 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]
