rambleraptor commented on code in PR #3478:
URL: https://github.com/apache/iceberg-python/pull/3478#discussion_r3983420378
##########
pyiceberg/table/deletion_vector.py:
##########
@@ -77,17 +89,103 @@ def to_vector(self) -> "pa.ChunkedArray":
return self._bitmaps_to_chunked_array(self._bitmaps)
-def _extract_vector_payload(blob_payload: bytes) -> bytes:
- """Strip deletion-vector-v1 blob framing: length(4 big-endian) + DV
magic(4) ... CRC(4 big-endian)."""
- length_prefix = int.from_bytes(blob_payload[0:4], "big")
- return blob_payload[8 : 4 + length_prefix]
+def _deserialize_dv_blob(blob: bytes, record_count: int | None = None) ->
list[BitMap]:
+ # The DV blob encoding matches Iceberg Java's BitmapPositionDeleteIndex:
+ # 4-byte big-endian bitmap-data length, 4-byte little-endian magic number,
+ # portable Roaring bitmap data, and 4-byte big-endian CRC-32.
+ if len(blob) < _DV_BLOB_MIN_SIZE_BYTES:
+ raise ValueError(f"Invalid deletion vector blob length: {len(blob)}")
+
+ bitmap_data_length = _DV_BLOB_LENGTH.unpack_from(blob)[0]
+ expected_bitmap_data_length = len(blob) - _DV_BLOB_LENGTH.size -
_DV_BLOB_CRC.size
+ if bitmap_data_length != expected_bitmap_data_length:
+ raise ValueError(f"Invalid bitmap data length: {bitmap_data_length},
expected {expected_bitmap_data_length}")
+
+ bitmap_data_offset = _DV_BLOB_LENGTH.size
+ crc_offset = bitmap_data_offset + bitmap_data_length
+ bitmap_data = blob[bitmap_data_offset:crc_offset]
+
+ magic_number = _DV_BLOB_MAGIC.unpack_from(bitmap_data)[0]
+ if magic_number != _DV_BLOB_MAGIC_NUMBER:
+ raise ValueError(f"Invalid magic number: {magic_number}, expected
{_DV_BLOB_MAGIC_NUMBER}")
+
+ checksum = zlib.crc32(bitmap_data) & 0xFFFFFFFF
+ expected_checksum = _DV_BLOB_CRC.unpack_from(blob, crc_offset)[0]
+ if checksum != expected_checksum:
+ raise ValueError("Invalid CRC")
+
+ bitmaps =
DeletionVector._deserialize_bitmap(bitmap_data[_DV_BLOB_MAGIC.size :])
+ if record_count is not None:
+ cardinality = sum(len(bitmap) for bitmap in bitmaps)
+ if cardinality != record_count:
+ raise ValueError(f"Invalid cardinality: {cardinality}, expected
{record_count}")
Review Comment:
Java does the check, so we should probably keep it just to match the
implementations. This is the kind of thing that I imagine
`iceberg-verification` will be checking at some point and we don't want to have
to add the check back in to help keep that repository green.
That being said, I always bias towards removing checks on user data, since
we can't always assume the writer did a valid job. It's such a waste to not
read a valid DV because of a mismatch.
##########
pyiceberg/table/__init__.py:
##########
@@ -2232,17 +2233,17 @@ class FileScanTask(ScanTask):
"""Task representing a data file and its corresponding delete files."""
file: DataFile
- delete_files: set[DataFile]
+ delete_files: DeleteFileSet
residual: BooleanExpression
def __init__(
self,
data_file: DataFile,
- delete_files: set[DataFile] | None = None,
+ delete_files: Iterable[DataFile] | None = None,
residual: BooleanExpression = ALWAYS_TRUE,
) -> None:
self.file = data_file
- self.delete_files = delete_files or set()
+ self.delete_files = DeleteFileSet(delete_files if delete_files is not
None else [])
Review Comment:
Doesn't look like you need the default, since DeleteFileSet already sets a
default.
--
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]
