moomindani commented on PR #3474:
URL: https://github.com/apache/iceberg-python/pull/3474#issuecomment-4800408288
### Reworked onto #3491
Now that #3491 has merged, I've rebased this onto it (via a merge of `main`
plus a rework commit, so the earlier history is preserved). The write path is
now split along the same format/domain boundary #3491 established on the read
side:
- **`DeletionVector` (domain)** gains the write-side counterparts of the
read path: `from_positions()`, `_serialize_bitmap()` (counterpart of
`_deserialize_bitmap()`), and `to_blob()` (counterpart of `to_vector()`). It
reuses `MAX_JAVA_SIGNED` / `PROPERTY_REFERENCED_DATA_FILE` from #3491.
- **`PuffinWriter` (format)** stays in `puffin.py` as a generic,
blob-agnostic writer that assembles a Puffin file from `PuffinBlob` payloads —
mirroring `PuffinFile` on the read side and scaling to future blob types (e.g.
`apache-datasketches-theta-v1`).
#### Notable changes in context
- **Naming**: I'd earlier said I'd rename to `DeletionVectorWriter`, but per
the follow-up discussion in #3491 we settled on keeping `PuffinWriter` generic
and putting the DV serialization on `DeletionVector` instead. That's what's
implemented here.
- **Interface**: `PuffinWriter` is now a context manager (consistent with
`ManifestWriter`); the file size is available via `len(output_file)` after the
`with` block, replacing the earlier `finish() -> int`.
- **`created-by`**: defaults to `PyIceberg version {__version__}` via `from
pyiceberg import __version__`.
#### How prior review feedback is addressed
| Feedback | Source | Status |
|---|---|---|
| `created_by` default = `PyIceberg version {version}` | @ebyhr | Done |
| Use `from pyiceberg import __version__` (not `importlib.metadata`) |
@sungwy | Done |
| Accept an `OutputFile` and write to it (Java `PuffinWriter` shape) |
@ebyhr | Done |
| Fix misleading docstring ("writer doesn't write a file") | @ebyhr | Done |
| Avoid numbered prefixes / excessive single-line comments | @ebyhr | Done |
| Extract the Spark interop test to its own PR | @ebyhr | Done (#3476) |
| DV serialization on `DeletionVector` (counterpart to `to_vector`) | #3491
| Done (`to_blob`) |
| Keep a generic, format-level `PuffinWriter` in `puffin.py` | #3491 | Done |
| Reuse `MAX_JAVA_SIGNED` / `PROPERTY_REFERENCED_DATA_FILE` | #3491 | Done |
| Lock in DV field id and blob framing with tests | review | Done
(`test_to_blob_payload_layout`) |
| Tests for sparse bitmap keys and the Java key-range limit | review | Done |
Wiring this into the merge-on-read branch of `Transaction.delete()` for v3
tables (toward #1078), where the Spark-reads-PyIceberg interop test will live,
remains a follow-up.
--
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]
