anxkhn opened a new pull request, #3660:
URL: https://github.com/apache/iceberg-python/pull/3660
# Rationale for this change
`ManifestWriter.to_manifest_file()` computed the manifest's minimum data
sequence number with a truthiness fallback:
```python
min_sequence_number = self._min_sequence_number or UNASSIGNED_SEQ
```
A data sequence number of `0` is a legitimate minimum for a live file (files
from a v1 table, or the initial commit of a v2 table). Because `0` is falsy,
`or` collapses it to `UNASSIGNED_SEQ` (`-1`), i.e. it treats a real `0` the
same
as "unset".
This diverges from the Java reference implementation, which falls back to
`UNASSIGNED_SEQ` only when the value is actually unset:
```java
// ManifestWriter#toManifestFile (apache/iceberg)
long minSeqNumber = minDataSequenceNumber != null ? minDataSequenceNumber :
UNASSIGNED_SEQ;
```
The `-1` is not harmless. When the manifest is produced by a merge/compaction
(so its `added_snapshot_id` equals the current commit snapshot id),
`ManifestListWriter.prepare_manifest()` treats `min_sequence_number ==
UNASSIGNED_SEQ` as "no file had an assigned sequence number" and overwrites
it
with the current, higher commit sequence number
(`pyiceberg/manifest.py`, the `if wrapped_manifest_file.min_sequence_number
==
UNASSIGNED_SEQ:` branch). The manifest's minimum data sequence number is
thereby
silently raised, which affects sequence-number-based delete-file application
and
scans.
The fix uses an explicit `None` check instead of a truthiness fallback,
mirroring the Java writer:
```python
min_sequence_number = self._min_sequence_number if self._min_sequence_number
is not None else UNASSIGNED_SEQ
```
## Are these changes tested?
Yes. A new parametrized regression test,
`tests/utils/test_manifest.py::test_write_manifest_min_sequence_number_zero`
(format versions 1 and 2), drives a live `EXISTING` entry with
`sequence_number=0` through `ManifestWriter.existing()` and asserts that
`to_manifest_file().min_sequence_number == 0`.
The test fails before the fix (`assert -1 == 0`) and passes after it. This
path
had no prior coverage: no existing test drove a live sequence-0 entry through
`to_manifest_file()`, which is why the defect was not caught.
Local runs (unit tests):
- `pytest tests/utils/test_manifest.py -q` -> passes.
- `pytest tests/table/test_snapshots.py tests/table/test_manage_snapshots.py
-q`
-> passes.
- `make lint` (ruff, ruff-format, mypy, pydocstyle, codespell) -> clean on
the
changed files.
The merge/compaction integration path exercised by `prepare_manifest()` is
covered by the integration suite, which requires Docker and Spark; those were
not run locally. The new unit test exercises the same `to_manifest_file()`
write
path that feeds it.
## Are there any user-facing changes?
No API changes. It is a correctness fix: manifests written for live files
whose
minimum data sequence number is `0` now record `0` instead of `-1`, so a
subsequent merge/compaction no longer silently raises the manifest's minimum
data sequence number.
--
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]
