From 4c58963dda58f6a333a15ebcc997189c766ce57b Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Wed, 6 Mar 2024 19:03:10 -0800 Subject: [PATCH 1/7] pass dict properties to set_properties --- tests/integration/test_reads.py | 36 +++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index da43e7825e..b739a35c95 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -152,6 +152,42 @@ def test_table_properties_error(catalog: Catalog) -> None: assert "Cannot pass both properties and kwargs" in str(e.value) +@pytest.mark.integration +@pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) +def test_table_properties_dict(catalog: Catalog) -> None: + table = create_table(catalog) + + assert table.properties == DEFAULT_PROPERTIES + + with table.transaction() as transaction: + transaction.set_properties({"abc": "🤪"}) + + assert table.properties == dict({"abc": "🤪"}, **DEFAULT_PROPERTIES) + + with table.transaction() as transaction: + transaction.remove_properties("abc") + + assert table.properties == DEFAULT_PROPERTIES + + table = table.transaction().set_properties({"abc": "def"}).commit_transaction() + + assert table.properties == dict({"abc": "def"}, **DEFAULT_PROPERTIES) + + table = table.transaction().remove_properties("abc").commit_transaction() + + assert table.properties == DEFAULT_PROPERTIES + + +@pytest.mark.integration +@pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) +def test_table_properties_error(catalog: Catalog) -> None: + table = create_table(catalog) + properties = {"abc": "def"} + with pytest.raises(ValueError) as e: + table.transaction().set_properties(properties, abc="def").commit_transaction() + assert "Cannot pass both properties and kwargs" in str(e.value) + + @pytest.mark.integration @pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) def test_pyarrow_nan(catalog: Catalog) -> None: From 3c858638379099f42279f35a31921a1536c6bd00 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Wed, 6 Mar 2024 20:25:32 -0800 Subject: [PATCH 2/7] whitespace --- tests/integration/test_reads.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index b739a35c95..f496d65bbe 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -161,20 +161,16 @@ def test_table_properties_dict(catalog: Catalog) -> None: with table.transaction() as transaction: transaction.set_properties({"abc": "🤪"}) - assert table.properties == dict({"abc": "🤪"}, **DEFAULT_PROPERTIES) with table.transaction() as transaction: transaction.remove_properties("abc") - assert table.properties == DEFAULT_PROPERTIES table = table.transaction().set_properties({"abc": "def"}).commit_transaction() - assert table.properties == dict({"abc": "def"}, **DEFAULT_PROPERTIES) table = table.transaction().remove_properties("abc").commit_transaction() - assert table.properties == DEFAULT_PROPERTIES From fee5ad90ba84381ca271f8149d66e73e257e12bd Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Wed, 6 Mar 2024 20:30:18 -0800 Subject: [PATCH 3/7] set non-string property values --- pyiceberg/table/__init__.py | 9 ++++++--- tests/integration/test_reads.py | 6 ++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index 09cb814a0e..07bf2587a0 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -42,7 +42,7 @@ Union, ) -from pydantic import Field, SerializeAsAny +from pydantic import Field, SerializeAsAny, field_validator from sortedcontainers import SortedList from typing_extensions import Annotated @@ -124,6 +124,7 @@ NestedField, PrimitiveType, StructType, + transform_dict_value_to_str, ) from pyiceberg.utils.concurrent import ExecutorFactory from pyiceberg.utils.datetime import datetime_to_millis @@ -293,7 +294,7 @@ def upgrade_table_version(self, format_version: Literal[1, 2]) -> Transaction: return self - def set_properties(self, properties: Properties = EMPTY_DICT, **kwargs: str) -> Transaction: + def set_properties(self, properties: Properties = EMPTY_DICT, **kwargs: Any) -> Transaction: """Set properties. When a property is already set, it will be overwritten. @@ -472,7 +473,9 @@ class SetLocationUpdate(TableUpdate): class SetPropertiesUpdate(TableUpdate): action: TableUpdateAction = TableUpdateAction.set_properties - updates: Dict[str, str] + updates: Properties + # validators + transform_properties_dict_value_to_str = field_validator('updates', mode='before')(transform_dict_value_to_str) class RemovePropertiesUpdate(TableUpdate): diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index f496d65bbe..701c19f068 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -119,6 +119,9 @@ def test_table_properties(catalog: Catalog) -> None: table = table.transaction().remove_properties("abc").commit_transaction() assert table.properties == DEFAULT_PROPERTIES + table = table.transaction().set_properties(abc=123).commit_transaction() + assert table.properties == dict(abc="123", **DEFAULT_PROPERTIES) + @pytest.mark.integration @pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) @@ -141,6 +144,9 @@ def test_table_properties_dict(catalog: Catalog) -> None: table = table.transaction().remove_properties("abc").commit_transaction() assert table.properties == DEFAULT_PROPERTIES + table = table.transaction().set_properties({"abc": 123}).commit_transaction() + assert table.properties == dict({"abc": "123"}, **DEFAULT_PROPERTIES) + @pytest.mark.integration @pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) From ee8aeabb21184020513bdd1609949b18e2d2d4ae Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Wed, 6 Mar 2024 20:33:34 -0800 Subject: [PATCH 4/7] test error for none --- tests/integration/test_reads.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index 701c19f068..e43c5db0fd 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -24,6 +24,7 @@ import pytest from hive_metastore.ttypes import LockRequest, LockResponse, LockState, UnlockRequest from pyarrow.fs import S3FileSystem +from pydantic_core import ValidationError from pyiceberg.catalog import Catalog, load_catalog from pyiceberg.catalog.hive import HiveCatalog, _HiveClient @@ -122,6 +123,10 @@ def test_table_properties(catalog: Catalog) -> None: table = table.transaction().set_properties(abc=123).commit_transaction() assert table.properties == dict(abc="123", **DEFAULT_PROPERTIES) + with pytest.raises(ValidationError) as exc_info: + table.transaction().set_properties(property_name=None).commit_transaction() + assert "None type is not a supported value in properties: property_name" in str(exc_info.value) + @pytest.mark.integration @pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) @@ -147,6 +152,10 @@ def test_table_properties_dict(catalog: Catalog) -> None: table = table.transaction().set_properties({"abc": 123}).commit_transaction() assert table.properties == dict({"abc": "123"}, **DEFAULT_PROPERTIES) + with pytest.raises(ValidationError) as exc_info: + table.transaction().set_properties({"property_name": None}).commit_transaction() + assert "None type is not a supported value in properties: property_name" in str(exc_info.value) + @pytest.mark.integration @pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) From 5616ef06171ae05dfa97518bdaf21eb395605052 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Wed, 6 Mar 2024 20:36:38 -0800 Subject: [PATCH 5/7] add comment --- tests/integration/test_reads.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index e43c5db0fd..c01f8f32d6 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -121,6 +121,7 @@ def test_table_properties(catalog: Catalog) -> None: assert table.properties == DEFAULT_PROPERTIES table = table.transaction().set_properties(abc=123).commit_transaction() + # properties are stored as strings in the iceberg spec assert table.properties == dict(abc="123", **DEFAULT_PROPERTIES) with pytest.raises(ValidationError) as exc_info: @@ -150,6 +151,7 @@ def test_table_properties_dict(catalog: Catalog) -> None: assert table.properties == DEFAULT_PROPERTIES table = table.transaction().set_properties({"abc": 123}).commit_transaction() + # properties are stored as strings in the iceberg spec assert table.properties == dict({"abc": "123"}, **DEFAULT_PROPERTIES) with pytest.raises(ValidationError) as exc_info: From 9fe9cff1241e105b5e0ecff314648aa00b8eb8cd Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Thu, 7 Mar 2024 07:55:06 -0800 Subject: [PATCH 6/7] rewrite validator --- pyiceberg/table/__init__.py | 6 ++++-- tests/integration/test_reads.py | 32 -------------------------------- 2 files changed, 4 insertions(+), 34 deletions(-) diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index 07bf2587a0..bb3b7afe33 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -474,8 +474,10 @@ class SetLocationUpdate(TableUpdate): class SetPropertiesUpdate(TableUpdate): action: TableUpdateAction = TableUpdateAction.set_properties updates: Properties - # validators - transform_properties_dict_value_to_str = field_validator('updates', mode='before')(transform_dict_value_to_str) + + @field_validator('updates', mode='before') + def transform_properties_dict_value_to_str(cls, properties: Properties) -> Dict[str, str]: + return transform_dict_value_to_str(properties) class RemovePropertiesUpdate(TableUpdate): diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index c01f8f32d6..fdc13ae752 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -169,38 +169,6 @@ def test_table_properties_error(catalog: Catalog) -> None: assert "Cannot pass both properties and kwargs" in str(e.value) -@pytest.mark.integration -@pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) -def test_table_properties_dict(catalog: Catalog) -> None: - table = create_table(catalog) - - assert table.properties == DEFAULT_PROPERTIES - - with table.transaction() as transaction: - transaction.set_properties({"abc": "🤪"}) - assert table.properties == dict({"abc": "🤪"}, **DEFAULT_PROPERTIES) - - with table.transaction() as transaction: - transaction.remove_properties("abc") - assert table.properties == DEFAULT_PROPERTIES - - table = table.transaction().set_properties({"abc": "def"}).commit_transaction() - assert table.properties == dict({"abc": "def"}, **DEFAULT_PROPERTIES) - - table = table.transaction().remove_properties("abc").commit_transaction() - assert table.properties == DEFAULT_PROPERTIES - - -@pytest.mark.integration -@pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) -def test_table_properties_error(catalog: Catalog) -> None: - table = create_table(catalog) - properties = {"abc": "def"} - with pytest.raises(ValueError) as e: - table.transaction().set_properties(properties, abc="def").commit_transaction() - assert "Cannot pass both properties and kwargs" in str(e.value) - - @pytest.mark.integration @pytest.mark.parametrize('catalog', [pytest.lazy_fixture('catalog_hive'), pytest.lazy_fixture('catalog_rest')]) def test_pyarrow_nan(catalog: Catalog) -> None: From 428b894aacb107547ba41433b4b6f37e1ad1914a Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Fri, 8 Mar 2024 21:52:12 -0800 Subject: [PATCH 7/7] properties validator --- pyiceberg/catalog/rest.py | 7 +++++-- pyiceberg/table/__init__.py | 2 +- pyiceberg/table/metadata.py | 4 +++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pyiceberg/catalog/rest.py b/pyiceberg/catalog/rest.py index c401339e18..a5f33f02dd 100644 --- a/pyiceberg/catalog/rest.py +++ b/pyiceberg/catalog/rest.py @@ -149,9 +149,12 @@ class CreateTableRequest(IcebergBaseModel): partition_spec: Optional[PartitionSpec] = Field(alias="partition-spec") write_order: Optional[SortOrder] = Field(alias="write-order") stage_create: bool = Field(alias="stage-create", default=False) - properties: Properties = Field(default_factory=dict) + properties: Dict[str, str] = Field(default_factory=dict) + # validators - transform_properties_dict_value_to_str = field_validator('properties', mode='before')(transform_dict_value_to_str) + @field_validator('properties', mode='before') + def transform_properties_dict_value_to_str(cls, properties: Properties) -> Dict[str, str]: + return transform_dict_value_to_str(properties) class RegisterTableRequest(IcebergBaseModel): diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index bb3b7afe33..76aa533d7c 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -473,7 +473,7 @@ class SetLocationUpdate(TableUpdate): class SetPropertiesUpdate(TableUpdate): action: TableUpdateAction = TableUpdateAction.set_properties - updates: Properties + updates: Dict[str, str] @field_validator('updates', mode='before') def transform_properties_dict_value_to_str(cls, properties: Properties) -> Dict[str, str]: diff --git a/pyiceberg/table/metadata.py b/pyiceberg/table/metadata.py index 1e5f0fdcec..323f6d85a8 100644 --- a/pyiceberg/table/metadata.py +++ b/pyiceberg/table/metadata.py @@ -221,7 +221,9 @@ class TableMetadataCommonFields(IcebergBaseModel): current-snapshot-id even if the refs map is null.""" # validators - transform_properties_dict_value_to_str = field_validator('properties', mode='before')(transform_dict_value_to_str) + @field_validator('properties', mode='before') + def transform_properties_dict_value_to_str(cls, properties: Properties) -> Dict[str, str]: + return transform_dict_value_to_str(properties) def snapshot_by_id(self, snapshot_id: int) -> Optional[Snapshot]: """Get the snapshot by snapshot_id."""