morningman opened a new pull request, #69076:
URL: https://github.com/apache/doris/pull/69076
### What problem does this PR solve?
Issue Number: None
Related PR: #59487
Problem Summary:
**In short.** The Iceberg `rewrite_manifests` procedure committed a snapshot
without rewriting any manifest, and still reported every targeted manifest as
rewritten. This PR clusters the data files by their first partition field, so
the rewrite actually merges manifests, and returns the numbers of manifests it
replaced and wrote. A unit test written for #68902 turned this up; branch-4.2
has the same code.
**Background.** `ALTER TABLE ... EXECUTE rewrite_manifests(['spec_id' =
...])` runs `RewriteManifestExecutor` on Iceberg's `RewriteManifests` API and
returns `rewritten_manifests_count` and `added_manifests_count`. That API
merges manifests only when a cluster function is set with `clusterBy`. Without
one, `BaseRewriteManifests.requiresRewrite` returns false and the commit keeps
every current manifest as it is.
**The problem, and what it cost.** The executor never set a cluster
function, so the procedure changed nothing while reporting that it had. On a
local single-BE cluster with a file:// Iceberg hadoop catalog:
| Table | master | with this PR |
|---|---|---|
| Unpartitioned, three INSERTs (3 manifests) | `[3, 0]`, still 3 manifests |
`[3, 1]`, 1 manifest |
| Partitioned by `(a, b)`, four INSERTs into `(1, x)`, `(1, y)`, `(2, x)`,
`(2, y)` (4 manifests) | `[4, 0]`, still 4 manifests | `[4, 2]`, 2 manifests,
one per value of `a` |
| One INSERT (1 small manifest) | `[1, 0]`, plus a new snapshot that changes
nothing | `[0, 0]`, no new snapshot |
Running the procedure to compact metadata never reduced the number of
manifests. Every run on a table with one manifest also added a snapshot.
**How this PR fixes it.**
- **Clustering.** The executor sets `clusterBy` to the value of the data
file's first partition field, as Trino's `optimize_manifests` does. A
multi-field partition is grouped by its first field only, so the rewrite does
not write one manifest per partition, and an unpartitioned table is one group.
Iceberg rolls each group over at `commit.manifest.target-size-bytes` (8 MB by
default). The key is the value that `PartitionData` returns rather than the
partition object, wrapped in a list because Iceberg rejects a null key, so a
NULL first partition value forms its own group.
- **Single small manifest.** As in Spark's `RewriteManifestsSparkAction` and
in Trino, a single targeted manifest smaller than the target size is left
alone: nothing is committed and the result is `[0, 0]`.
- **Counts.** `rewritten_manifests_count` is now the number of targeted
manifests that the rewrite replaced, and `added_manifests_count` the number of
manifests it wrote: the difference between the manifest lists before and after
the commit.
- `spec_id` still limits the rewrite to the manifests of one spec, and
delete manifests are kept as they are, as Iceberg's rewrite does.
Spark's action instead sorts every entry by partition and splits the entries
into `ceil(total size / target size)` manifests. In Spark that sort is
distributed, but here the FE would have to read every entry into memory first,
so this PR uses Iceberg's streaming cluster function, as Trino does. As with
Trino, a rewrite produces at least one manifest per value of the first
partition field, so a table with many such values but few manifests can end up
with more manifests than before.
**Results.**
- **FE UT.** Five cases in `IcebergRewriteManifestsActionTest` fail on
master and pass with this PR:
- three appends become one manifest, `[3, 1]`;
- a two-field partition is clustered by its first field, `[4, 2]`;
- two partitions that one commit wrote into the same manifest are split by
value, `[2, 2]`;
- a NULL first partition value gets its own manifest, `[3, 2]`;
- a single small manifest is left alone, `[0, 0]`, with no new snapshot.
On master the first four return `[N, 0]` and the last returns `[1, 0]`.
Keying by the partition object as Trino does fails the NULL case (`Key cannot
be null`). All 113 tests under `org.apache.doris.connector.iceberg.action` and
the `IcebergProcedureOps` tests pass.
- **End to end.** The table above was produced on a local cluster. After the
rewrite, the three tables still return 3, 4 and 1 rows.
- **Regression.** `test_iceberg_rewrite_manifests` is unchanged. Its `.out`
records only data rows, and its count checks still hold with the new counts: at
least one manifest rewritten, no more added than rewritten, no more manifests
after than before, and the exact rewritten count for `spec_id = 0`. The docker
environment was not running locally, so it was not run here.
### Release note
Iceberg `rewrite_manifests` now merges manifests, clustered by the first
partition field, and returns the numbers of manifests it actually replaced and
wrote. A table with a single manifest smaller than
`commit.manifest.target-size-bytes` is left unchanged.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- `IcebergRewriteManifestsActionTest`: the five cases above, which
fail on master; all 113 tests under `org.apache.doris.connector.iceberg.action`
and the `IcebergProcedureOps` tests pass.
- [x] Manual test (add detailed scripts or steps below)
- Local single-BE cluster with a file:// Iceberg hadoop catalog: the
three tables in the table above, before and after this change.
- [ ] 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 -->
- `rewrite_manifests` rewrites manifests, clustered by the first
partition field, and reports the manifests it replaced and wrote; a single
small manifest is left alone instead of being committed as a new snapshot.
- 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]
