syun64 commented on code in PR #555:
URL: https://github.com/apache/iceberg-python/pull/555#discussion_r1544015221
##########
pyiceberg/table/__init__.py:
##########
@@ -2526,25 +2537,44 @@ def _dataframe_to_data_files(
"""
from pyiceberg.io.pyarrow import bin_pack_arrow_table, write_file
- if len([spec for spec in table_metadata.partition_specs if spec.spec_id !=
0]) > 0:
- raise ValueError("Cannot write to partitioned tables")
-
counter = itertools.count(0)
write_uuid = write_uuid or uuid.uuid4()
-
target_file_size = PropertyUtil.property_as_int(
properties=table_metadata.properties,
property_name=TableProperties.WRITE_TARGET_FILE_SIZE_BYTES,
default=TableProperties.WRITE_TARGET_FILE_SIZE_BYTES_DEFAULT,
)
+ if target_file_size is None:
+ raise ValueError(
+ "Fail to get neither TableProperties.WRITE_TARGET_FILE_SIZE_BYTES
nor WRITE_TARGET_FILE_SIZE_BYTES_DEFAULT for writing target data file."
Review Comment:
I have mixed feelings about this exception check, because we are setting the
default value of `target_file_size` as
TableProperties.WRITE_TARGET_FILE_SIZE_BYTES_DEFAULT right in the previous
line. I feel as though this is too redundant.
I understand why we are doing it though:
`PropertyUtil.property_as_int` returns `Optional[int]`, and bin_packing
expects an int, so we need to type check it.
If we run into more of these type checking redundancies in the code base,
where when we are using property values that are always expected to have a
none-null default value, maybe we should refactor `PropertyUtil` instead. Maybe
we can have two methods, `property_as_int` that returns an `Optional[int]`, and
`property_as_int_with_default`, that returns an `int`?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
