Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions pyiceberg/io/pyarrow.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,10 @@
visit_with_partner,
)
from pyiceberg.table import DOWNCAST_NS_TIMESTAMP_TO_US_ON_WRITE, TableProperties
from pyiceberg.table.deletion_vector import deletion_vectors_from_puffin_file
from pyiceberg.table.deletion_vector import read_deletion_vectors
from pyiceberg.table.locations import load_location_provider
from pyiceberg.table.metadata import TableMetadata
from pyiceberg.table.name_mapping import NameMapping, apply_name_mapping
from pyiceberg.table.puffin import PuffinFile
from pyiceberg.transforms import IdentityTransform, TruncateTransform
from pyiceberg.typedef import EMPTY_DICT, Properties, Record, TableVersion
from pyiceberg.types import (
Expand Down Expand Up @@ -1139,10 +1138,7 @@ def _read_deletes(io: FileIO, data_file: DataFile) -> dict[str, pa.ChunkedArray]
for path in table.column("file_path").unique()
}
elif data_file.file_format == FileFormat.PUFFIN:
with io.new_input(data_file.file_path).open() as fi:
payload = fi.read()

return {dv.referenced_data_file: dv.to_vector() for dv in deletion_vectors_from_puffin_file(PuffinFile(payload))}
return {dv.referenced_data_file: dv.to_vector() for dv in read_deletion_vectors(io, data_file)}
else:
raise ValueError(f"Delete file format not supported: {data_file.file_format}")

Expand Down
12 changes: 12 additions & 0 deletions pyiceberg/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,18 @@ def equality_ids(self) -> list[int] | None:
def sort_order_id(self) -> int | None:
return self._data[15]

@property
def referenced_data_file(self) -> str | None:
return self._data[17] if len(self._data) > 17 else None

@property
def content_offset(self) -> int | None:
return self._data[18] if len(self._data) > 18 else None

@property
def content_size_in_bytes(self) -> int | None:
return self._data[19] if len(self._data) > 19 else None

# Spec ID should not be stored in the file
_spec_id: int

Expand Down
103 changes: 98 additions & 5 deletions pyiceberg/table/deletion_vector.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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]:

Copy link
Copy Markdown
Collaborator

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.

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:

Copy link
Copy Markdown
Collaborator

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 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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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
]
18 changes: 14 additions & 4 deletions tests/avro/test_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
LongType,
NestedField,
StringType,
StructType,
TimestampType,
TimestamptzType,
TimeType,
Expand Down Expand Up @@ -89,7 +90,16 @@ def test_missing_schema() -> None:

# helper function to serialize our objects to dicts to enable
# direct comparison with the dicts returned by fastavro
def todict(obj: Any) -> Any:
def todict(obj: Any, struct: StructType | None = None) -> Any:
if struct is not None and isinstance(obj, Record):
return {
field.name: todict(
getattr(obj, field.name),
field.field_type if isinstance(field.field_type, StructType) else None,
)
for field in struct.fields
if hasattr(obj, field.name)
}
if isinstance(obj, dict):
data = []
for k, v in obj.items():
Expand Down Expand Up @@ -157,7 +167,7 @@ def test_write_manifest_entry_with_iceberg_read_with_fastavro_v1() -> None:

fa_entry = next(it)

v2_entry = todict(entry)
v2_entry = todict(entry, MANIFEST_ENTRY_SCHEMAS[2].as_struct())

# These are not written in V1
del v2_entry["sequence_number"]
Expand Down Expand Up @@ -222,7 +232,7 @@ def test_write_manifest_entry_with_iceberg_read_with_fastavro_v2() -> None:

fa_entry = next(it)

assert todict(entry) == fa_entry
assert todict(entry, MANIFEST_ENTRY_SCHEMAS[2].as_struct()) == fa_entry


@pytest.mark.parametrize("format_version", [1, 2])
Expand Down Expand Up @@ -260,7 +270,7 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta
schema = AvroSchemaConversion().iceberg_to_avro(MANIFEST_ENTRY_SCHEMAS[format_version], schema_name="manifest_entry")

with open(tmp_avro_file, "wb") as out:
writer(out, schema, [todict(entry)])
writer(out, schema, [todict(entry, MANIFEST_ENTRY_SCHEMAS[format_version].as_struct())])

# Read as V2
with avro.AvroFile[ManifestEntry](
Expand Down
21 changes: 18 additions & 3 deletions tests/integration/test_rest_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,28 @@
from pyiceberg.avro.codecs import AvroCompressionCodec
from pyiceberg.catalog import Catalog, load_catalog
from pyiceberg.io.pyarrow import PyArrowFileIO
from pyiceberg.manifest import DataFile, write_manifest
from pyiceberg.manifest import MANIFEST_ENTRY_SCHEMAS, DataFile, write_manifest
from pyiceberg.table import Table
from pyiceberg.typedef import Record
from pyiceberg.types import StructType
from pyiceberg.utils.lazydict import LazyDict


# helper function to serialize our objects to dicts to enable
# direct comparison with the dicts returned by fastavro
def todict(obj: Any, spec_keys: list[str]) -> Any:
def todict(obj: Any, spec_keys: list[str], struct: StructType | None = None) -> Any:
if type(obj) is Record:
return {key: obj[pos] for key, pos in zip(spec_keys, range(len(obj)), strict=True)}
if struct is not None and isinstance(obj, Record):
return {
field.name: todict(
getattr(obj, field.name),
spec_keys,
field.field_type if isinstance(field.field_type, StructType) else None,
)
for field in struct.fields
if hasattr(obj, field.name)
}
if isinstance(obj, dict) or isinstance(obj, LazyDict):
data = []
for k, v in obj.items():
Expand Down Expand Up @@ -111,7 +122,11 @@ def test_write_sample_manifest(table_test_all_types: Table, compression: AvroCom
)
wrapped_entry_v2 = copy(entry)
wrapped_entry_v2.data_file = wrapped_data_file_v2_debug
wrapped_entry_v2_dict = todict(wrapped_entry_v2, [field.name for field in test_spec.fields])
wrapped_entry_v2_dict = todict(
wrapped_entry_v2,
[field.name for field in test_spec.fields],
MANIFEST_ENTRY_SCHEMAS[2].as_struct(),
)

with TemporaryDirectory() as tmpdir:
tmp_avro_file = tmpdir + "/test_write_manifest.avro"
Expand Down
Loading