Do not refuse a manifest-only Iceberg rewrite over an absent parent total - #117012
Do not refuse a manifest-only Iceberg rewrite over an absent parent total#117012groeneai wants to merge 1 commit into
Conversation
|
Workflow [PR], commit [0572538] AI ReviewSummaryThis PR changes Missing context / blind spots
Tests
Final Verdict |
…otal The `total-*` snapshot summary metrics are optional per the Iceberg spec, so a table written by another engine may omit them and still read correctly. `set_data_total` threw BAD_ARGUMENTS whenever the parent summary omitted one, regardless of the delta being applied. `generateManifestOnlySnapshot` passes 0 for every delta, so `OPTIMIZE TABLE ... MANIFEST` was permanently unavailable on such a table: Code: 36. DB::Exception: Cannot derive Iceberg snapshot total 'total-records': the parent snapshot's summary omits it. (BAD_ARGUMENTS) That contradicts the documented contract of `generateManifestOnlySnapshot`, which is to carry the `total-*` counters forward so the rewrite is idempotent. Condition the refusal on the delta rather than on the caller. A zero delta leaves the table-wide total exactly as the parent had it, so an absent counter is carried forward by staying absent, the way the sibling `set_delete_total` already tolerates absence. A non-zero delta still cannot be derived from an unknown total, so the INSERT path keeps refusing: writing the delta alone would report a total that omits the parent's rows. Verified at SQL level on an `IcebergLocal` table whose current snapshot summary has `total-records` removed: before, `OPTIMIZE TABLE t MANIFEST` failed with the error above while the table read correctly; after, it succeeds and the new snapshot omits `total-records` while forwarding `total-files-size` and `total-data-files` unchanged. An untouched control table compacts identically in both cases, which is what shows the fixture reaches this code rather than an earlier threshold check. Two tests are added to the existing gtest module, one per branch of the new condition. Making the carry-forward unconditional instead does not compile (-Werror,-Wunreachable-code), so the over-broad form of this change is rejected by the build. Closes: ClickHouse#117000
6edf795 to
0572538
Compare
| /// engine may omit them. A manifest-only rewrite adds no data, so every such total is unchanged and | ||
| /// an absent one stays absent; refusing the rewrite would leave OPTIMIZE ... MANIFEST permanently | ||
| /// unavailable on a table that reads correctly. Totals the parent does carry are still forwarded. | ||
| TEST(IcebergMetadataGenerator, ManifestOnlySnapshotWhenParentTotalRecordsMissing) |
There was a problem hiding this comment.
The implementation change looks reasonable, but this only exercises MetadataGenerator in isolation. The regression in #117000 reproduces on the SQL OPTIMIZE TABLE ... MANIFEST path against on-disk Iceberg metadata, and the repo already has manifest-compaction integration coverage in tests/integration/test_storage_iceberg_with_spark/test_manifest_compaction.py. Please add a focused end-to-end case there (or a stateless IcebergLocal test) that strips total-records / total-files-size / total-data-files from the latest metadata file and runs the actual OPTIMIZE, so future caller-side regressions are caught too.
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units7 translation units recompiled, 12 s compile time in total, 7 of them have a recent master baseline. |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixes
OPTIMIZE TABLE ... MANIFESTfailing withBAD_ARGUMENTSon an Iceberg table whose current snapshot summary omits an optionaltotal-records,total-files-sizeortotal-data-filesmetric, which made manifest compaction permanently unavailable on tables written by engines that omit those metrics. Closes #117000.Description
Closes: #117000
The
total-*snapshot summary metrics are optional per the Iceberg spec, so a table written by another engine may omit them and still read correctly.setSnapshotTotalsrefused to derive a new total whenever the parent summary omitted one, regardless of the delta being applied.generateManifestOnlySnapshotpasses 0 for every delta, soOPTIMIZE TABLE ... MANIFESTwas refused outright:That also contradicts the documented contract of
generateManifestOnlySnapshot, which is to carry thetotal-*counters forward so the rewrite is idempotent.I made the refusal depend on the delta rather than on the caller. A zero delta leaves the total exactly as the parent had it, so an absent counter is carried forward by staying absent, as the sibling delete-family totals already tolerate absence. A non-zero delta still cannot be derived from an unknown total, so the INSERT path keeps refusing.
The new stateless test strips
total-recordsfrom anIcebergLocaltable's current snapshot summary, reloads the table and runs the real statement. Before,OPTIMIZE TABLE t MANIFESTfailed as above while the table read correctly; after, it succeeds and the new snapshot omitstotal-recordswhile forwardingtotal-files-sizeandtotal-data-filesunchanged. Widening the new condition to accept any delta instead fails theINSERTarm, so each arm covers a different defect. The existingtest_optimize_manifest_parent_summary_missing_totalsstrips only the three delete-family counters, whichset_delete_totalalready tolerated, which is why this case was uncovered.Note for review: on this path an absent delete-family counter is still published as
0even when delete files are live. Master behaves identically, and that test pins the0, so I left it out of scope rather than widen this fix.Workflow [PR]
Sync PR [sync-upstream/pr/117012]