morningman opened a new pull request, #69043:
URL: https://github.com/apache/doris/pull/69043
### What problem does this PR solve?
Issue Number: None
Related PR: #68902
Problem Summary:
**In short.** The Iceberg `expire_snapshots` procedure counted every expired
manifest as a deleted data file, so `deleted_data_files_count` was too high and
`deleted_manifest_files_count` was almost always 0. This came up in a unit test
written for #68902.
**Background.** `expire_snapshots` returns the six counters of Spark's
`ExpireSnapshotsSparkAction`: deleted data files, position delete files,
equality delete files, manifests, manifest lists and statistics files.
`IcebergExpireSnapshotsAction` fills them in the `deleteWith` callback that
Iceberg calls for each file it removes. Delete files are recognized by path,
from a map the action builds from every snapshot's delete manifests. Manifests
and manifest lists were recognized by name: a path that contains `-m-` and ends
in `.avro` was a manifest, one that contains `snap-` was a manifest list, and
anything else that is not a statistics file was a data file.
**The problem, and what it cost.** Iceberg names a manifest `<commit
UUID>-m<N>.avro` (`SnapshotProducer.newManifestOutputFile`), so a manifest
written by a normal commit never contains `-m-`; only the `optimized-m-` names
of `rewrite_manifests` did. Every manifest that `expire_snapshots` deleted was
therefore counted as a data file. For example, after appending `f1`, deleting
it and appending `f2`, `expire_snapshots("retain_last" = "1")` reclaims `f1`,
two manifests and two manifest lists:
| Counter | master | with this PR |
|---|---|---|
| `deleted_data_files_count` | 3 | 1 |
| `deleted_manifest_files_count` | 0 | 2 |
| `deleted_manifest_lists_count` | 2 | 2 |
Only the reported counts were wrong; the files that Iceberg deletes are the
same.
**How this PR fixes it.** The callback classifies manifests and manifest
lists by path, as it already does for delete files. While the action reads
every snapshot's manifest list to find the delete files, it now also records
the paths of all manifests (`Snapshot.allManifests`, which the same read
already loaded) and every snapshot's `manifestListLocation()`. A single map
from path to file kind replaces the delete-file content map. Only statistics
files are still told apart by name.
**Results.**
`IcebergExpireSnapshotsActionTest.countsExpiredManifestsAsManifestFiles` builds
the table above and asserts `[1, 0, 0, 2, 2, 0]`; on master it gets `[3, 0, 0,
0, 2, 0]`. The existing tests keep their results, including the append-only
case (`[0, 0, 0, 0, 2, 0]`) and the delete-manifest dedup. The regression
`test_iceberg_expire_snapshots` records `0 0 0 0 2 0` for an append-only table,
where no manifest expires, so its output does not change.
### Release note
Fix the counters of the Iceberg `expire_snapshots` procedure, which counted
expired manifests as deleted data files.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
-
`IcebergExpireSnapshotsActionTest.countsExpiredManifestsAsManifestFiles` (new;
fails on master as shown above); all 85 tests under
`org.apache.doris.connector.iceberg.action` and the 25 `IcebergProcedureOps`
tests pass.
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
- `expire_snapshots` counts expired manifests in
`deleted_manifest_files_count` instead of `deleted_data_files_count`.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
