Skip to content

Fix duplicate deletion vectors when filter pushdown clones the multi file list#1054

Merged
Tishj merged 8 commits into
duckdb:v1.5-variegatafrom
NiclasHaderer:nh/dynamic-pushdown-deletion-bug
Jun 13, 2026
Merged

Fix duplicate deletion vectors when filter pushdown clones the multi file list#1054
Tishj merged 8 commits into
duckdb:v1.5-variegatafrom
NiclasHaderer:nh/dynamic-pushdown-deletion-bug

Conversation

@NiclasHaderer

Copy link
Copy Markdown
Contributor

Some Iceberg DML operators (IcebergDelete) capture a reference to the IcebergMultiFileList in the scan's bind data at planning time. If filters are pushed into the scan afterwards (e.g. runtime join min/max filters during UPDATE ... FROM), PushdownInternal creates a clone with empty delete-state maps, and the scan reads existing deletes into the clone. FlushDeletes then consults the original's empty map, misses the table's existing deletion vector, and neither invalidates its manifest entry nor merges its positions. The commit ends up with two DVs for the same data file, and the next read fails with Table is corrupt, two or more deletion vectors exist for the same referenced_data_file.

Fix: keep positional_delete_data and equality_delete_data in a shared_ptr and alias them in PushdownInternal, so a list and all its pushdown clones share one delete state regardless of which instance scans and which one flushes. Filter state stays per-instance. Sharing needs no additional locking because the accesses are ordered by the pipeline: deletes are written while the scan runs, and FlushDeletes only reads them in the sink's Finalize, which the scheduler fires after the scan has completed.

NiclasHaderer added a commit to motherduckdb/duckdb-iceberg that referenced this pull request Jun 11, 2026
@Tishj

Tishj commented Jun 11, 2026

Copy link
Copy Markdown
Member

This might be a different solution to #1038

Could you include the test from that PR here as well to verify?

@NiclasHaderer
NiclasHaderer force-pushed the nh/dynamic-pushdown-deletion-bug branch from 9f37fff to c05091a Compare June 11, 2026 11:20
@NiclasHaderer

Copy link
Copy Markdown
Contributor Author

Just added the test and ran it locally. Seems to work

@NiclasHaderer
NiclasHaderer marked this pull request as ready for review June 11, 2026 15:16
filtered_list->have_bound = true;
//! Share the delete state: DML operators hold a reference to the original list of the scan's bind data,
//! while the scan itself can read through a filtered copy created by (dynamic) filter pushdown.
filtered_list->positional_delete_data = positional_delete_data;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there might be a lifetime issue here, because the map eventually references a IcebergManifestEntry entry, which can be owned by this initial instance and isn't properly transferred

I think we just need to rework this a bit to group all of the gathered metadata into a shared instance.
The member that is of significance to this issue is likely the mutable vector<IcebergManifestListEntry> committed_delete_manifests;

@Tishj
Tishj merged commit f2a075c into duckdb:v1.5-variegata Jun 13, 2026
25 of 26 checks passed
lnkuiper added a commit to duckdb/duckdb that referenced this pull request Jun 15, 2026
krlmlr pushed a commit to krlmlr/duckdb that referenced this pull request Jun 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants