kevinjqliu commented on code in PR #3954:
URL: https://github.com/apache/iceberg-python/pull/3954#discussion_r3997191411
##########
pyiceberg/manifest.py:
##########
@@ -464,9 +464,14 @@ def data_file_with_partition(partition_type: StructType,
format_version: TableVe
class DataFile(Record):
@classmethod
- def from_args(cls, _table_format_version: TableVersion =
DEFAULT_READ_VERSION, **arguments: Any) -> DataFile:
+ def from_args(
+ cls, _table_format_version: TableVersion = DEFAULT_READ_VERSION, *,
spec_id: int | None = None, **arguments: Any
+ ) -> DataFile:
struct = DATA_FILE_TYPE[_table_format_version]
- return super()._bind(struct, **arguments)
+ data_file = super()._bind(struct, **arguments)
+ if spec_id is not None:
+ data_file.spec_id = spec_id
+ return data_file
Review Comment:
this is using DataFile's own setter
https://github.com/apache/iceberg-python/blob/562d3af3a77476bcd7b923f31da60200c1b2563c/pyiceberg/manifest.py#L551-L560
##########
pyiceberg/manifest.py:
##########
@@ -464,9 +464,14 @@ def data_file_with_partition(partition_type: StructType,
format_version: TableVe
class DataFile(Record):
@classmethod
- def from_args(cls, _table_format_version: TableVersion =
DEFAULT_READ_VERSION, **arguments: Any) -> DataFile:
+ def from_args(
+ cls, _table_format_version: TableVersion = DEFAULT_READ_VERSION, *,
spec_id: int | None = None, **arguments: Any
Review Comment:
explicitly add `spec_id` to the function.
its called in
-
https://github.com/apache/iceberg-python/blob/308768d99b67816434e8d5c6dfea278737430ef8/pyiceberg/io/pyarrow.py#L2771
-
https://github.com/apache/iceberg-python/blob/308768d99b67816434e8d5c6dfea278737430ef8/pyiceberg/io/pyarrow.py#L2909
I think this is better. Otherwise caller has to set it after constructing
DataFile. For example
```
data_file = DataFile.from_args()
data_file.spec_id = spec_id
```
##########
tests/avro/test_file.py:
##########
@@ -289,7 +288,11 @@ def
test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta
avro_entry = next(it)
if format_version == 1:
- data_file_v1 = DataFile.from_args(**data_file_dict,
_table_format_version=format_version)
+ data_file_v1 = DataFile.from_args(
+ _table_format_version=format_version,
+ block_size_in_bytes=DEFAULT_BLOCK_SIZE,
Review Comment:
`block_size_in_bytes` is required in V1.
##########
pyiceberg/typedef.py:
##########
@@ -179,6 +179,9 @@ class Record(StructProtocol):
@classmethod
def _bind(cls, struct: StructType, **arguments: Any) -> Self:
+ field_names = {field.name for field in struct.fields}
Review Comment:
including this fix as part of the PR.
This is the footgun that was silently dropping fields. We now check for
unknown fields.
This caught `format_version=2,` in `tests/integration/test_rest_manifest.py`
which is invalid
##########
tests/integration/test_rest_manifest.py:
##########
@@ -112,7 +111,7 @@ def test_write_sample_manifest(table_test_all_types: Table,
compression: AvroCom
wrapped_entry_v2 = copy(entry)
wrapped_entry_v2.data_file = wrapped_data_file_v2_debug
wrapped_entry_v2_dict = todict(wrapped_entry_v2, [field.name for field in
test_spec.fields])
- for field in ("first_row_id", "referenced_data_file", "content_offset",
"content_size_in_bytes"):
+ for field in ("first_row_id", "referenced_data_file", "content_offset",
"content_size_in_bytes", "spec_id"):
Review Comment:
`spec_id` is not serialized so we need to remove it for this comparison to
work
##########
tests/avro/test_file.py:
##########
@@ -248,16 +247,16 @@ def
test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta
"upper_bounds": {1: b"zzzzzzzzzzzzzzzz"},
"key_metadata": b"\xde\xad\xbe\xef",
"split_offsets": [4, 133697593],
- "equality_ids": [],
"sort_order_id": 4,
"spec_id": 3,
}
- data_file_v2 = DataFile.from_args(**data_file_dict) # type: ignore
+ data_file = DataFile.from_args(content=DataFileContent.DATA,
**common_data_file_args) # type: ignore
Review Comment:
`content` is a V2 field.
`common_data_file_args` is shared between v1 and v2 in this test file
--
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]
