Fokko commented on code in PR #3478:
URL: https://github.com/apache/iceberg-python/pull/3478#discussion_r4204112335
##########
pyiceberg/table/deletion_vector.py:
##########
@@ -77,17 +89,99 @@ 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}")
+
+ return bitmaps
+
+
+def _validate_deletion_vector_content(dv: "DataFile") -> None:
+ content_offset = dv.content_offset
+ content_size_in_bytes = dv.content_size_in_bytes
+ referenced_data_file = dv.referenced_data_file
+
+ if content_offset is None:
+ raise ValueError(f"Invalid deletion vector, content offset is missing:
{dv.file_path}")
+ if content_size_in_bytes is None:
+ raise ValueError(f"Invalid deletion vector, content size is missing:
{dv.file_path}")
+ if content_offset < 0:
+ raise ValueError(f"Invalid deletion vector, content offset cannot be
negative: {content_offset}")
+ if content_size_in_bytes < 0:
+ raise ValueError(f"Invalid deletion vector, content size cannot be
negative: {content_size_in_bytes}")
+ if content_size_in_bytes > _MAX_DELETION_VECTOR_CONTENT_SIZE:
+ raise ValueError(f"Cannot read deletion vector larger than 2GB:
{content_size_in_bytes}")
+ if referenced_data_file is None:
+ raise ValueError(f"Invalid deletion vector, referenced data file is
missing: {dv.file_path}")
+
+
+def has_deletion_vector_content_reference(dv: "DataFile") -> bool:
+ """Return whether a deletion vector is described by manifest content-range
metadata."""
+ return dv.content_offset is not None or dv.content_size_in_bytes is not
None or dv.referenced_data_file is not None
Review Comment:
I think these should be `and`, rather than `or`, since we require them all
downstream in `_read_deletion_vector`
##########
pyiceberg/table/delete_file.py:
##########
@@ -0,0 +1,96 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from collections.abc import Iterable, Iterator, MutableSet
+from dataclasses import dataclass
+from typing import Any
+
+from pyiceberg.manifest import DataFile
+
+
+@dataclass(frozen=True, slots=True)
+class DeleteFileKey:
+ """Identity of a delete file, including its referenced content range."""
+
+ file_path: str
+ content_offset: int | None
+ content_size_in_bytes: int | None
+
+ @classmethod
+ def from_file(cls, delete_file: DataFile) -> DeleteFileKey:
+ """Create a key from a delete file."""
+ return cls(
+ file_path=delete_file.file_path,
+ content_offset=delete_file.content_offset,
+ content_size_in_bytes=delete_file.content_size_in_bytes,
+ )
+
+
+class DeleteFileSet(MutableSet[DataFile]):
Review Comment:
I'm wondering if we could make it extend `Set`, rather than `MutableSet`. We
don't used `discard` and `update` does an `add` operation. I think having this
as immutable, that it makes it easier to reason about the code and flow.
##########
pyiceberg/table/delete_file.py:
##########
@@ -0,0 +1,96 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from collections.abc import Iterable, Iterator, MutableSet
+from dataclasses import dataclass
+from typing import Any
+
+from pyiceberg.manifest import DataFile
+
+
+@dataclass(frozen=True, slots=True)
Review Comment:
Created an issue for it: https://github.com/apache/iceberg-python/issues/4086
##########
pyiceberg/table/delete_file.py:
##########
@@ -0,0 +1,96 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from collections.abc import Iterable, Iterator, MutableSet
+from dataclasses import dataclass
+from typing import Any
+
+from pyiceberg.manifest import DataFile
+
+
+@dataclass(frozen=True, slots=True)
+class DeleteFileKey:
+ """Identity of a delete file, including its referenced content range."""
+
+ file_path: str
+ content_offset: int | None
+ content_size_in_bytes: int | None
+
+ @classmethod
+ def from_file(cls, delete_file: DataFile) -> DeleteFileKey:
+ """Create a key from a delete file."""
+ return cls(
+ file_path=delete_file.file_path,
+ content_offset=delete_file.content_offset,
+ content_size_in_bytes=delete_file.content_size_in_bytes,
+ )
+
+
+class DeleteFileSet(MutableSet[DataFile]):
+ """Set-like delete-file collection keyed by location and content range."""
+
+ _files: dict[DeleteFileKey, DataFile]
+
+ def __init__(self, delete_files: Iterable[DataFile] = ()) -> None:
+ self._files = {}
+ for delete_file in delete_files:
+ self.add(delete_file)
+
+ def __contains__(self, delete_file: object) -> bool:
+ """Return whether the delete file is present."""
+ return isinstance(delete_file, DataFile) and
DeleteFileKey.from_file(delete_file) in self._files
+
+ def __iter__(self) -> Iterator[DataFile]:
+ """Return an iterator over delete files."""
+ return iter(self._files.values())
+
+ def __len__(self) -> int:
+ """Return the number of delete files."""
+ return len(self._files)
+
+ def add(self, delete_file: DataFile) -> None:
+ self._files.setdefault(DeleteFileKey.from_file(delete_file),
delete_file)
+
+ def discard(self, delete_file: DataFile) -> None:
+ self._files.pop(DeleteFileKey.from_file(delete_file), None)
+
+ def update(self, delete_files: Iterable[DataFile]) -> None:
+ for delete_file in delete_files:
+ self.add(delete_file)
+
+ def __repr__(self) -> str:
+ """Return a string representation of the delete file set."""
+ return f"{type(self).__name__}({list(self)!r})"
+
+ def __eq__(self, other: Any) -> bool:
+ """Compare delete file sets by delete file identity."""
+ if isinstance(other, DeleteFileSet):
+ return self._files.keys() == other._files.keys()
+
+ if not isinstance(other, Iterable):
+ return False
+
+ other_keys: set[DeleteFileKey] = set()
+ other_count = 0
+ for delete_file in other:
+ if not isinstance(delete_file, DataFile):
+ return False
+ other_keys.add(DeleteFileKey.from_file(delete_file))
+ other_count += 1
+
+ return len(other_keys) == other_count and set(self._files) ==
other_keys
Review Comment:
This part feels odd to me, why do we want to compare this to an iterable?
##########
pyiceberg/table/delete_file.py:
##########
@@ -0,0 +1,96 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from collections.abc import Iterable, Iterator, MutableSet
+from dataclasses import dataclass
+from typing import Any
+
+from pyiceberg.manifest import DataFile
+
+
+@dataclass(frozen=True, slots=True)
Review Comment:
Love the `slots=True`. We define slots by hand throughout the codebase, but
that's not needed anymore with Python 3.10
--
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]
