[ghoshp83]

Closes #4291.

Rationale for this change

A partial Table.delete rewrites each data file that might hold matching rows. It reads the whole file with AlwaysTrue() and writes back the rows that don't match. The files to rewrite were planned with the delete filter, though, and that plan also leaves out delete files whose column bounds can't match the filter. For a filtered read that's correct, because the residual drops those rows anyway. For a full-file rewrite it isn't: the left-out delete files are never applied, so the rows they deleted come back in the new data file.

overwrite(..., overwrite_filter=...), dynamic_partition_overwrite and upsert go through Transaction.delete, so they're affected too.

The fix keeps the delete filter for choosing data files, but stops pruning delete files by their column bounds when planning a rewrite:

• ManifestGroupPlanner takes prune_delete_files: bool = True. When it's False, the metrics evaluator is applied to data entries only. Partition pruning is unchanged, since a delete file only applies to data files in its own partition.
• Transaction.delete plans its rewrite with prune_delete_files=False. Its scan never has a catalog, so it always planned locally, and server-side planning isn't involved.

Scans keep the default, so reads and their planning cost are unchanged.

Are these changes tested?

Yes. test_partial_delete_keeps_earlier_deletes_outside_the_filter in tests/catalog/test_catalog_behaviors.py runs against each catalog fixture and follows case 1 from the issue:

• it commits a position delete that stores the deleted row (2, 'b'), which gives it bounds k = 2;
• it checks that a scan for k = 1 leaves that delete file out;
• it runs delete("k = 1") and checks only k = 3 is left.

Without the fix the final assertion fails with [2, 3] == [3]: the deleted row is back. PyIceberg can't write delete manifests yet, so the test's helper subclasses ManifestWriterV2, as the reproduction in the issue does. Once #4002 lands, the helper can pass content=ManifestContent.DELETES instead.

The equality-delete case from the issue isn't tested here, because PyIceberg can't read equality deletes. With this change, a partial delete over a file that has an equality delete now reaches the existing "does not yet support equality deletes" error instead of silently writing the deleted rows back.

Are there any user-facing changes?

Partial deletes, filtered overwrites and upserts no longer resurrect rows removed by earlier delete files. A partial delete on a table with equality delete files that only the column bounds used to exclude now raises the existing unsupported-equality-deletes error, rather than writing incorrect data.

Not in this PR: the issue also points out that the retry conflict check (_filter_manifest_entries in table/update/validate.py) prunes delete entries the same way. That's a separate code path, so I'll follow up on it separately.

[AP24110011695]

Verified locally (Windows, Python 3.12):

• test_partial_delete_keeps_earlier_deletes_outside_the_filter passes on this branch across all three catalog fixtures, and fails on the pre-change code with the exact resurrection the issue describes ([2, 3] vs [3]) — reproduced by swapping only the pyiceberg/ source tree.
• Regression slice on this branch: full tests/catalog/test_catalog_behaviors.py + tests/table/test_delete_file_index.py, 721 passed.
• Checked the two behavioral risks I was worried about: (1) the direct ManifestGroupPlanner construction drops scan options, but the only consumed option is include_empty_files, and Transaction._scan never set options anyway — so both paths see EMPTY_DICT. (2) Skipping DataScan.plan_files() also skips the server-side planning branch, but Transaction._scan passes no catalog, so _should_use_server_side_planning() was always false here — the claim in the description holds.
• The keep_delete_files wrapper threads into _open_manifest correctly and sequence-number manifest filtering is unchanged; data entries are still metrics-pruned, so rewrite targeting is unaffected.

Good catch on the asymmetry between filtered reads (where bounds-pruning delete files is safe) and full-file rewrites (where it silently resurrects rows) — and for fixing someone else's issue rather than leaving it. Nice work.

[Fokko]

Oof, this is a big one @ghoshp83 Thanks for fixing this! I left a comment to simplify it, can you also fix merge conflicts?

[Fokko]

Since a manifest holds either data or delete files, could we pick the evaluator per manifest instead of checking content on every entry? That also avoids having both evaluate_metrics and metrics_evaluator:

metrics_evaluator = self._build_metrics_evaluator()
...
    (
        self.io,
        manifest,
        partition_evaluators[manifest.partition_spec_id],
        metrics_evaluator if self.prune_delete_files or manifest.content == ManifestContent.DATA else lambda _: True,
    )

[ghoshp83]

Done, thanks! Switched to picking the evaluator per manifest, and rebased on main to clear the conflicts.

[kevinjqliu]

nit: can we use something like

        _ignore_residuals=True,

instead? It follows the repo's conventions more closely:

• The _ prefix keeps it out of the public API, like _isolation_operation; ManifestGroupPlanner shipped in 0.12.0.
• It matches Java's ignoreResiduals: the caller says files are read in full, and the planner keeps every delete file and sets residuals to always true.

[ghoshp83]

Done, renamed to ignore_residuals (defaulting to False).

[AP24110011695]

Follow-up verification on the two new commits (7b10913 + d334371):

• The evaluator is now picked per manifest (metrics_evaluator if self.prune_delete_files or manifest.content == ManifestContent.DATA else lambda _: True inside the manifest loop), matching the suggested shape — the per-entry content check and the two competing evaluators are gone.
• The rename to ignore_residuals (default False) is complete — no stale prune_delete_files references remain in the diff.
• Ran tests/table/test_upsert.py, tests/table/test_delete_file_index.py and tests/table/test_evaluator_planning.py on head d334371 locally: 22 passed, and the 17 failures are the identical set that fails on pristine main in my environment (Windows/datafusion warnings baseline) — so no regressions from this PR, and the four new upsert tests all pass.

The NaN-partition predicate addition (IsNaN for float partition values in upsert equality) also reads correctly alongside it.