You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/04 10:03:17 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #5192: Python: Move PartitionSpec to Pydantic
Fokko opened a new pull request, #5192:
URL: https://github.com/apache/iceberg/pull/5192
Blocked by https://github.com/apache/iceberg/pull/5124
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5192:
URL: https://github.com/apache/iceberg/pull/5192#discussion_r920339027
##########
python/tests/table/test_metadata.py:
##########
@@ -205,13 +207,13 @@ def test_updating_metadata():
def test_serialize_v1():
table_metadata = TableMetadataV1(**EXAMPLE_TABLE_METADATA_V1).json()
- expected = """{"location": "s3://bucket/test/location", "table-uuid": "d20125c8-7284-442c-9aea-15fee620737c", "last-updated-ms": 1602638573874, "last-column-id": 3, "schemas": [{"fields": [{"id": 1, "name": "x", "type": "long", "required": true}, {"id": 2, "name": "y", "type": "long", "required": true, "doc": "comment"}, {"id": 3, "name": "z", "type": "long", "required": true}], "schema-id": 0, "identifier-field-ids": []}], "current-schema-id": 0, "partition-specs": [{"spec-id": 0, "fields": [{"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000}]}], "default-spec-id": 0, "last-partition-id": 1000, "properties": {}, "snapshots": [{"snapshot-id": 1925, "timestamp-ms": 1602638573822}], "snapshot-log": [], "metadata-log": [], "sort-orders": [{"order-id": 0, "fields": []}], "default-sort-order-id": 0, "refs": {}, "format-version": 1, "schema": {"fields": [{"id": 1, "name": "x", "type": "long", "required": true}, {"id": 2, "name": "y", "type": "long", "required": tr
ue, "doc": "comment"}, {"id": 3, "name": "z", "type": "long", "required": true}], "schema-id": 0, "identifier-field-ids": []}, "partition-spec": [{"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000}]}"""
+ expected = """{"location": "s3://bucket/test/location", "table-uuid": "d20125c8-7284-442c-9aea-15fee620737c", "last-updated-ms": 1602638573874, "last-column-id": 3, "schemas": [{"fields": [{"id": 1, "name": "x", "type": "long", "required": true}, {"id": 2, "name": "y", "type": "long", "required": true, "doc": "comment"}, {"id": 3, "name": "z", "type": "long", "required": true}], "schema-id": 0, "identifier-field-ids": []}], "current-schema-id": 0, "partition-specs": [{"spec-id": 0, "fields": [{"source-id": 1, "field-id": 1000, "transform": "identity", "name": "x"}], "last-assigned-field-id": 1000}], "default-spec-id": 0, "last-partition-id": 1000, "properties": {}, "snapshots": [{"snapshot-id": 1925, "timestamp-ms": 1602638573822}], "snapshot-log": [], "metadata-log": [], "sort-orders": [{"order-id": 0, "fields": []}], "default-sort-order-id": 0, "refs": {}, "format-version": 1, "schema": {"fields": [{"id": 1, "name": "x", "type": "long", "required": true}, {"id": 2, "name": "y"
, "type": "long", "required": true, "doc": "comment"}, {"id": 3, "name": "z", "type": "long", "required": true}], "schema-id": 0, "identifier-field-ids": []}, "partition-spec": [{"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000}]}"""
Review Comment:
Serialization should not include `last-partition-id`.
--
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
[GitHub] [iceberg] rdblue merged pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5192:
URL: https://github.com/apache/iceberg/pull/5192
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5192:
URL: https://github.com/apache/iceberg/pull/5192#discussion_r920331948
##########
python/pyiceberg/table/partitioning.py:
##########
@@ -57,24 +81,33 @@ class PartitionSpec:
last_assigned_field_id(int): auto-increment partition field id starting from PARTITION_DATA_ID_START
"""
- schema: Schema
- spec_id: int
- fields: Tuple[PartitionField, ...]
- last_assigned_field_id: int
- source_id_to_fields_map: Dict[int, List[PartitionField]] = field(init=False, repr=False)
-
- def __post_init__(self):
- source_id_to_fields_map = {}
- for partition_field in self.fields:
- source_column = self.schema.find_column_name(partition_field.source_id)
- if not source_column:
- raise ValueError(f"Cannot find source column: {partition_field.source_id}")
- existing = source_id_to_fields_map.get(partition_field.source_id, [])
- existing.append(partition_field)
- source_id_to_fields_map[partition_field.source_id] = existing
- object.__setattr__(self, "source_id_to_fields_map", source_id_to_fields_map)
-
- def __eq__(self, other):
+ spec_id: int = Field(alias="spec-id")
+ fields: Tuple[PartitionField, ...] = Field()
+ last_assigned_field_id: int = Field(alias="last-assigned-field-id", default=_PARTITION_DATA_ID_START)
Review Comment:
We probably don't need this. It is just a high watermark of the field IDs in this spec. We can just define a property that returns it:
```python
@property
def last_assigned_field_id(self):
return max(pf.field_id for pf in self.fields)
```
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5192:
URL: https://github.com/apache/iceberg/pull/5192#discussion_r919031187
##########
python/pyiceberg/transforms.py:
##########
@@ -91,7 +91,7 @@ def validate(cls, v: Any):
elif v.startswith(BUCKET):
return BucketTransform(num_buckets=BUCKET_PARSER.match(v))
elif v.startswith(TRUNCATE):
- return TruncateTransform(width=BUCKET_PARSER.match(v))
+ return TruncateTransform(width=TRUNCATE_PARSER.match(v))
Review Comment:
Oops!
--
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
[GitHub] [iceberg] Fokko commented on pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #5192:
URL: https://github.com/apache/iceberg/pull/5192#issuecomment-1182487898
@rdblue Feel like some more Python?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5192:
URL: https://github.com/apache/iceberg/pull/5192#discussion_r920331948
##########
python/pyiceberg/table/partitioning.py:
##########
@@ -57,24 +81,33 @@ class PartitionSpec:
last_assigned_field_id(int): auto-increment partition field id starting from PARTITION_DATA_ID_START
"""
- schema: Schema
- spec_id: int
- fields: Tuple[PartitionField, ...]
- last_assigned_field_id: int
- source_id_to_fields_map: Dict[int, List[PartitionField]] = field(init=False, repr=False)
-
- def __post_init__(self):
- source_id_to_fields_map = {}
- for partition_field in self.fields:
- source_column = self.schema.find_column_name(partition_field.source_id)
- if not source_column:
- raise ValueError(f"Cannot find source column: {partition_field.source_id}")
- existing = source_id_to_fields_map.get(partition_field.source_id, [])
- existing.append(partition_field)
- source_id_to_fields_map[partition_field.source_id] = existing
- object.__setattr__(self, "source_id_to_fields_map", source_id_to_fields_map)
-
- def __eq__(self, other):
+ spec_id: int = Field(alias="spec-id")
+ fields: Tuple[PartitionField, ...] = Field()
+ last_assigned_field_id: int = Field(alias="last-assigned-field-id", default=_PARTITION_DATA_ID_START)
Review Comment:
We probably don't need this. It is just a high watermark of the field IDs in this spec. We can just define a property that returns it:
```python
@property
def last_assigned_field_id(self):
return max(pf.field_id for pf in self.fields)
```
This isn't serialized with the partition spec.
--
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
[GitHub] [iceberg] Fokko commented on pull request #5192: Python: Move PartitionSpec to Pydantic
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #5192:
URL: https://github.com/apache/iceberg/pull/5192#issuecomment-1183534288
@rdblue thanks, I've removed it as a field, and added it as a property:
```python
@property
def last_assigned_field_id(self) -> int:
if self.fields:
return max(pf.field_id for pf in self.fields)
return _PARTITION_DATA_ID_START
```
--
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