You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kevinjqliu (via GitHub)" <gi...@apache.org> on 2024/03/07 04:49:03 UTC
[PR] Kevinjqliu/set non string properties [iceberg-python]
kevinjqliu opened a new pull request, #504:
URL: https://github.com/apache/iceberg-python/pull/504
Requires #503 to be merged first.
This PR modifies `Transaction`'s `set_properties` API to accept table properties as type `Dict[str, Any]`, either as dictionary or kwargs.
For example,
```
with table.transaction() as transaction:
transaction.set_properties(abc=123)
transaction.set_properties({"abc": 123})
```
This is done by reusing the field validator defined in #469. Setting `None` property value is disallowed since the transformation will change None to str "None" which is unintuitive.
The Iceberg spec stores table properties as strings. That is why when setting a property value as int, reading it back produces a string.
```
table = table.transaction().set_properties({"abc": 123}).commit_transaction()
assert table.properties == dict({"abc": "123"}, **DEFAULT_PROPERTIES)
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Allow setting non-string typed values in `set_properties` [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #504:
URL: https://github.com/apache/iceberg-python/pull/504#discussion_r1515657781
##########
pyiceberg/table/__init__.py:
##########
@@ -294,17 +295,21 @@ def upgrade_table_version(self, format_version: Literal[1, 2]) -> Transaction:
return self
- def set_properties(self, **updates: str) -> Transaction:
+ def set_properties(self, properties: Properties = EMPTY_DICT, **kwargs: Any) -> Transaction:
Review Comment:
Just copy my comment here https://github.com/apache/iceberg-python/issues/502#issuecomment-1982550509
##########
pyiceberg/table/__init__.py:
##########
@@ -469,7 +474,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)
Review Comment:
```suggestion
@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)
```
Shall we reformat the validator to use decorator? This aligns with the way we define model validators in `metadata.py` and also the example in [Pydantic doc](https://docs.pydantic.dev/latest/concepts/validators/#field-validators). WDYT?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Allow setting non-string typed values in `set_properties` [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #504:
URL: https://github.com/apache/iceberg-python/pull/504#discussion_r1518482846
##########
pyiceberg/table/__init__.py:
##########
@@ -472,7 +473,11 @@ class SetLocationUpdate(TableUpdate):
class SetPropertiesUpdate(TableUpdate):
action: TableUpdateAction = TableUpdateAction.set_properties
- updates: Dict[str, str]
+ updates: Properties
Review Comment:
I think this one should be `Dict[str, str]` because at the time when the model is constructed, the before validator has converted all values to str
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Allow setting non-string typed values in `set_properties` [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #504:
URL: https://github.com/apache/iceberg-python/pull/504#issuecomment-1985996781
rebased after #503
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Allow setting non-string typed values in `set_properties` [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #504:
URL: https://github.com/apache/iceberg-python/pull/504
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Allow setting non-string typed values in `set_properties` [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #504:
URL: https://github.com/apache/iceberg-python/pull/504#discussion_r1516413608
##########
pyiceberg/table/__init__.py:
##########
@@ -469,7 +474,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)
Review Comment:
looks much cleaner, thank you!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org