-
Notifications
You must be signed in to change notification settings - Fork 526
Support range-based reads for deletion vectors #3478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| import math | ||
| import struct | ||
| import zlib | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| from pyroaring import BitMap, FrozenBitMap | ||
|
|
@@ -24,9 +26,19 @@ | |
| if TYPE_CHECKING: | ||
| import pyarrow as pa | ||
|
|
||
| from pyiceberg.io import FileIO | ||
| from pyiceberg.manifest import DataFile | ||
|
|
||
| EMPTY_BITMAP = FrozenBitMap() | ||
| MAX_JAVA_SIGNED = int(math.pow(2, 31)) - 1 | ||
| PROPERTY_REFERENCED_DATA_FILE = "referenced-data-file" | ||
| _MAX_DELETION_VECTOR_CONTENT_SIZE = 2**31 - 1 | ||
| _DV_BLOB_LENGTH = struct.Struct(">I") | ||
| _DV_BLOB_MAGIC = struct.Struct("<I") | ||
| _DV_BLOB_CRC = struct.Struct(">I") | ||
| _DV_BLOB_MAGIC_NUMBER = 1681511377 | ||
| _ROARING_BITMAP_COUNT_SIZE_BYTES = 8 | ||
| _DV_BLOB_MIN_SIZE_BYTES = _DV_BLOB_LENGTH.size + _DV_BLOB_MAGIC.size + _ROARING_BITMAP_COUNT_SIZE_BYTES + _DV_BLOB_CRC.size | ||
|
|
||
|
|
||
| class DeletionVector: | ||
|
|
@@ -77,17 +89,98 @@ 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(data_file: "DataFile") -> tuple[int, int, str]: | ||
| content_offset = data_file.content_offset | ||
| content_size_in_bytes = data_file.content_size_in_bytes | ||
| referenced_data_file = data_file.referenced_data_file | ||
|
|
||
| if content_offset is None: | ||
| raise ValueError(f"Invalid deletion vector, content offset is missing: {data_file.file_path}") | ||
| if content_size_in_bytes is None: | ||
| raise ValueError(f"Invalid deletion vector, content size is missing: {data_file.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: {data_file.file_path}") | ||
|
|
||
| return content_offset, content_size_in_bytes, referenced_data_file | ||
|
|
||
|
|
||
| def _has_deletion_vector_content_reference(data_file: "DataFile") -> bool: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about making this a public method? |
||
| return ( | ||
| data_file.content_offset is not None | ||
| or data_file.content_size_in_bytes is not None | ||
| or data_file.referenced_data_file is not None | ||
| ) | ||
|
|
||
|
|
||
| def read_deletion_vector(io: "FileIO", data_file: "DataFile") -> DeletionVector: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it should be a private method. We should make it exceptionally clear which methods we expect users to call, especially when the two of them have the same list of parameters. The list method isn't just calling the non-list method in a loop, which is what most people would expect. |
||
| content_offset, content_size_in_bytes, referenced_data_file = _validate_deletion_vector_content(data_file) | ||
|
|
||
| with io.new_input(data_file.file_path).open() as fi: | ||
| fi.seek(content_offset) | ||
| payload = fi.read(content_size_in_bytes) | ||
|
|
||
| if len(payload) != content_size_in_bytes: | ||
| raise ValueError(f"Could not read deletion vector, expected {content_size_in_bytes} bytes, got {len(payload)}") | ||
|
|
||
| return DeletionVector( | ||
| referenced_data_file=referenced_data_file, | ||
| bitmaps=_deserialize_dv_blob(payload, data_file.record_count), | ||
| ) | ||
|
|
||
|
|
||
| def read_deletion_vectors(io: "FileIO", data_file: "DataFile") -> list[DeletionVector]: | ||
| if _has_deletion_vector_content_reference(data_file): | ||
| return [read_deletion_vector(io, data_file)] | ||
|
|
||
| with io.new_input(data_file.file_path).open() as fi: | ||
| return deletion_vectors_from_puffin_file(PuffinFile(fi.read())) | ||
|
|
||
|
|
||
| def deletion_vectors_from_puffin_file(puffin_file: PuffinFile) -> list[DeletionVector]: | ||
| return [ | ||
| DeletionVector( | ||
| referenced_data_file=blob.properties[PROPERTY_REFERENCED_DATA_FILE], | ||
| bitmaps=DeletionVector._deserialize_bitmap(_extract_vector_payload(puffin_file.get_blob_payload(blob))), | ||
| bitmaps=_deserialize_dv_blob(puffin_file.get_blob_payload(blob)), | ||
| ) | ||
| for blob in puffin_file.footer.blobs | ||
| ] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having this function just handle validation and not have it return anything?
The values you're returning aren't hard to parse, it'll increase readability, and gives this function just one job.