-
Notifications
You must be signed in to change notification settings - Fork 530
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
Open
KaiqiJinWow
wants to merge
1
commit into
apache:main
Choose a base branch
from
KaiqiJinWow:fix-dv-content-range-read
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+332
−20
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,9 @@ | |
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| import math | ||
| from typing import TYPE_CHECKING | ||
| import struct | ||
| import zlib | ||
| from typing import TYPE_CHECKING, cast | ||
|
|
||
| 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,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}") | ||
|
Comment on lines
+131
to
+136
Contributor
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. I am fine with having a more defensive implementation (the spec requires writers to produce the offset/size/refereenced file for DVs anyways) but just mentioning i think we only need to do these checks once and in one place only rather than in multiple places. |
||
| 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 | ||
|
|
||
|
|
||
| def _read_deletion_vector(io: "FileIO", dv: "DataFile") -> DeletionVector: | ||
| _validate_deletion_vector_content(dv) | ||
|
|
||
| content_offset = cast(int, dv.content_offset) | ||
| content_size_in_bytes = cast(int, dv.content_size_in_bytes) | ||
| referenced_data_file = cast(str, dv.referenced_data_file) | ||
|
|
||
| with io.new_input(dv.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, dv.record_count), | ||
| ) | ||
|
|
||
|
|
||
| def read_deletion_vectors(io: "FileIO", dv: "DataFile") -> list[DeletionVector]: | ||
| """Read deletion vectors from a delete file or its manifest content range.""" | ||
| if has_deletion_vector_content_reference(dv): | ||
| return [_read_deletion_vector(io, dv)] | ||
|
|
||
| with io.new_input(dv.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]: | ||
| """Read all deletion vectors stored in a Puffin file.""" | ||
| 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 | ||
| ] | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this is fine, again as we expect these two values to be the same but just remember implementations can choose to be a bit more relaxed (or vice versa more strict) than the actual spec. Is it worth failing the read of the DV if there's a mismatch? On one hand it indicates something incorrect in the metadata, on the other hand, we could be blocking a read of the data unnecessarily (because it wouldn't affect correctness of the result anyways). So in this case I'd probably bias to the latter of not doing this check. But I'll leave it up to you cc @kevinjqliu @rambleraptor in case you folks have opinions here.