fix: preserve manifest min sequence number of 0#3660
Open
anxkhn wants to merge 1 commit into
Open
Conversation
ManifestWriter.to_manifest_file() used `self._min_sequence_number or UNASSIGNED_SEQ`, which treats a legitimate minimum data sequence number of 0 as falsy and collapses it to UNASSIGNED_SEQ (-1). A data sequence number of 0 is valid for a live file (files from a v1 table, or the initial commit of a v2 table), so this diverges from the Java reference, which falls back to UNASSIGNED_SEQ only when the minimum is unset (null). When such a manifest is produced by a merge/compaction (the added snapshot id equals the current commit snapshot id), prepare_manifest() interprets the -1 as "no file had an assigned sequence number" and overwrites it with the current, higher commit sequence number, silently raising the manifest's min data sequence number. That in turn affects sequence-number-based delete-file application and scans. Use an explicit None check, mirroring the Java ManifestWriter, and add a regression test that drives a live existing entry with sequence number 0 through to_manifest_file() for both format versions. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Rationale for this change
ManifestWriter.to_manifest_file()computed the manifest's minimum datasequence number with a truthiness fallback:
A data sequence number of
0is a legitimate minimum for a live file (filesfrom a v1 table, or the initial commit of a v2 table). Because
0is falsy,orcollapses it toUNASSIGNED_SEQ(-1), i.e. it treats a real0the sameas "unset".
This diverges from the Java reference implementation, which falls back to
UNASSIGNED_SEQonly when the value is actually unset:The
-1is not harmless. When the manifest is produced by a merge/compaction(so its
added_snapshot_idequals the current commit snapshot id),ManifestListWriter.prepare_manifest()treatsmin_sequence_number == UNASSIGNED_SEQas "no file had an assigned sequence number" and overwrites itwith the current, higher commit sequence number
(
pyiceberg/manifest.py, theif wrapped_manifest_file.min_sequence_number == UNASSIGNED_SEQ:branch). The manifest's minimum data sequence number is therebysilently raised, which affects sequence-number-based delete-file application and
scans.
The fix uses an explicit
Nonecheck instead of a truthiness fallback,mirroring the Java writer:
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
EXISTINGentry withsequence_number=0throughManifestWriter.existing()and asserts thatto_manifest_file().min_sequence_number == 0.The test fails before the fix (
assert -1 == 0) and passes after it. This pathhad 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 thechanged files.
The merge/compaction integration path exercised by
prepare_manifest()iscovered by the integration suite, which requires Docker and Spark; those were
not run locally. The new unit test exercises the same
to_manifest_file()writepath 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
0now record0instead of-1, so asubsequent merge/compaction no longer silently raises the manifest's minimum
data sequence number.