Fix Iceberg trivial count bug#111617
Conversation
… summary IcebergMetadata::totalRows() returned the total-records hint from the snapshot summary unconditionally. Writers maintain that hint as parent_total + added, so it silently diverges from the actual data when the table history contains a lossy or corrupted commit, making SELECT count() disagree with a full scan of the same table. Derive the count from the manifest list instead: in format v2+ every entry carries added_rows_count/existing_rows_count, whose sum over data manifests is the exact live row count. Fall back to the summary hint only for v1 manifest lists without counts or when delete manifests are present. Log a warning when the summary disagrees with the manifest list.
|
Workflow [PR], commit [96694c9] Summary: ⏳
AI ReviewSummaryThis PR makes Iceberg Findings❌ Blockers
Final Verdict❌ Changes requested. |
| /// status ADDED and EXISTING respectively. Required in format v2+, optional in v1 | ||
| /// (std::nullopt when absent or null). Their sum over all data manifests is the exact | ||
| /// live row count of a snapshot without delete files, see IcebergMetadata::totalRows(). | ||
| std::optional<Int64> added_rows_count; |
| } | ||
| ], | ||
| "properties": { | ||
| "owner": "divanik", |
There was a problem hiding this comment.
Legacy via copy-paste
| if (entry.content_type == Iceberg::ManifestFileContentType::DATA) | ||
| { | ||
| if (entry.added_rows_count.has_value() && entry.existing_rows_count.has_value()) | ||
| manifest_list_rows += *entry.added_rows_count + *entry.existing_rows_count; |
There was a problem hiding this comment.
why
manifest_list_rows += *entry.added_rows_count + *entry.existing_rows_count
?
I think
manifest_list_rows += *entry.added_rows_count
There was a problem hiding this comment.
Because EXISTING manifests are still part of the current snapshot. After the first commit, a snapshot can inherit manifests from earlier snapshots (plain append, manifest-only rewrite, compaction, etc.), and those rows remain live even though they were not newly added in this snapshot. Summing only added_rows_count would undercount every snapshot that carries forward pre-existing manifests.
|
@melvynator we merged some PRs about summary when we do different operations on iceberg tables, and another one on review #109288, with some tests for mutations as well. I think we are testing summary snapshot consistency much better now and the corruption chance by clickhouse is less then before, but what if counters are wrong in manifests lists? should we check the data files then? We would again ended up with broken count. WDYT about adding some metadata validation on iceberg tables creation/attaching:
cc: @scanhex12 |
…ests - Use UInt64 for the manifest-list row counts (review comment), treating negative values as absent since they violate the spec. - Do not fall back to the summary total-records hint when the snapshot has delete manifests: the hint is maintained incrementally by writers and can be corrupted the same way, so go to the manifest scan, which derives both data and position-delete rows from the actual manifests. - Add a stateless test covering position deletes with a corrupted summary: 5 rows, 2 deleted via merge-on-read, total-records corrupted from 5 to 100; count() must return 3, not 98.
| /// poisons it the same way, and with delete manifests present the manifest scan below | ||
| /// is the only source that derives both data and position-delete rows from the actual | ||
| /// manifests instead of writer-provided hints. | ||
| if (!has_delete_manifests && summary_total_rows.has_value()) |
There was a problem hiding this comment.
This branch sends every delete-manifest snapshot into the manifest scan below, but that scan only subtracts POSITION_DELETE rows. We already have a fixture showing why that is not enough: 03595_equality_deletes_simple.reference has count(name) = 881 for deletes_db/eq_deletes_table, while the current snapshot metadata says total-records = 1010 and total-position-deletes = 30. If totalRows() runs through the code below, it comes back as 980 because equality deletes are never accounted for.
Please bail out of trivial count when any manifest file contains EQUALITY_DELETE entries (return {} and force a real scan), or add logic that can prove the exact number of rows removed by equality deletes.
|
|
||
| /// Row counts are required in v2+ manifest lists and optional in v1. | ||
| /// They let totalRows() compute an exact count without opening manifest files. | ||
| /// Negative values violate the spec, treat them as absent. |
There was a problem hiding this comment.
For v2 manifest lists these row-count fields are required, so treating a negative or NULL value as std::nullopt is not just "count unavailable" - it changes the control flow in IcebergMetadata::totalRows(). A corrupted v2 entry lands in all_data_manifests_have_counts = false, and the next branch at IcebergMetadata.cpp:1172 goes straight back to summary_total_rows, which is exactly the stale writer-provided hint this PR is trying to stop trusting.
Please either throw ICEBERG_SPECIFICATION_VIOLATION for malformed v2 row counts, or at least make the invalid-v2 case bypass the summary fast path and fall through to the manifest-file scan.
This is addressing the bug of trivial count mentioned in #92861
When the summary in the snapshot get corrupted by a bad commit it might get out of sync compared to the real count.
Changelog category (leave one):
Changelog entry (a [user-readable short description]
Fix Iceberg trivial count in the case where the snapshot summary is not accurate by referring to the manifest list.