Iceberg table maintenance Phase 3: multi-spec compaction, delete handling, and metrics (#8643)

* Add multi-partition-spec compaction and delete-aware compaction (Phase 3)

Multi-partition-spec compaction:
- Add SpecID to compactionBin struct and group by spec+partition key
- Remove the len(specIDs) > 1 skip that blocked spec-evolved tables
- Write per-spec manifests in compaction commit using specByID map
- Use per-bin PartitionSpec when calling NewDataFileBuilder

Delete-aware compaction:
- Add ApplyDeletes config (default: true) with readBoolConfig helper
- Implement position delete collection (file_path + pos Parquet columns)
- Implement equality delete collection (field ID to column mapping)
- Update mergeParquetFiles to filter rows via position deletes (binary
  search) and equality deletes (hash set lookup)
- Smart delete manifest carry-forward: drop when all data files compacted
- Fix EXISTING/DELETED entries to include sequence numbers

Tests for multi-spec bins, delete collection, merge filtering, and
end-to-end compaction with position/equality/mixed deletes.

* Add structured metrics and per-bin progress to iceberg maintenance

- Change return type of all four operations from (string, error) to
  (string, map[string]int64, error) with structured metric counts
  (files_merged, snapshots_expired, orphans_removed, duration_ms, etc.)
- Add onProgress callback to compactDataFiles for per-bin progress
- In Execute, pass progress callback that sends JobProgressUpdate with
  per-bin stage messages
- Accumulate per-operation metrics with dot-prefixed keys
  (e.g. compact.files_merged) into OutputValues on completion
- Update testing_api.go wrappers and integration test call sites
- Add tests: TestCompactDataFilesMetrics, TestExpireSnapshotsMetrics,
  TestExecuteCompletionOutputValues

* Address review feedback: group equality deletes by field IDs, use metric constants

- Group equality deletes by distinct equality_ids sets so different
  delete files with different equality columns are handled correctly
- Use length-prefixed type-aware encoding in buildEqualityKey to avoid
  ambiguity between types and collisions from null bytes
- Extract metric key strings into package-level constants

* Fix buildEqualityKey to use length-prefixed type-aware encoding

The previous implementation used plain String() concatenation with null
byte separators, which caused type ambiguity (int 123 vs string "123")
and separator collisions when values contain null bytes. Now each value
is serialized as "kind:length:value" for unambiguous composite keys.

This fix was missed in the prior cherry-pick due to a merge conflict.

* Address nitpick review comments

- Document patchManifestContentToDeletes workaround: explain that
  iceberg-go WriteManifest cannot create delete manifests, and note
  the fail-fast validation on pattern match
- Document makeTestEntries: note that specID field is ignored and
  callers should use makeTestEntriesWithSpec for multi-spec testing

* fmt

* Fix path normalization, manifest threshold, and artifact filename collisions

- Normalize file paths in position delete collection and lookup so that
  absolute S3 URLs and relative paths match correctly
- Fix rewriteManifests threshold check to count only data manifests
  (was including delete manifests in the count and metric)
- Add random suffix to artifact filenames in compactDataFiles and
  rewriteManifests to prevent collisions between concurrent runs
- Sort compaction bins by SpecID then PartitionKey for deterministic
  ordering across specs

* Fix pos delete read, deduplicate column resolution, minor cleanups

- Remove broken Column() guard in position delete reading that silently
  defaulted pos to 0; unconditionally extract Int64() instead
- Deduplicate column resolution in readEqualityDeleteFile by calling
  resolveEqualityColIndices instead of inlining the same logic
- Add warning log in readBoolConfig for unrecognized string values
- Fix CompactDataFiles call site in integration test to capture 3 return
  values

* Advance progress on all bins, deterministic manifest order, assert metrics

- Call onProgress for every bin iteration including skipped/failed bins
  so progress reporting never appears stalled
- Sort spec IDs before iterating specEntriesMap to produce deterministic
  manifest list ordering across runs
- Assert expected metric keys in CompactDataFiles integration test

---------

Co-authored-by: Copilot <copilot@github.com>
This commit is contained in:
Chris Lu
2026-03-15 19:18:14 -07:00
committed by GitHub
parent e24630251c
commit a00eddb525
8 changed files with 2004 additions and 176 deletions

View File

@@ -2,13 +2,18 @@ package iceberg
import (
"bytes"
"context"
"fmt"
"io"
"path"
"testing"
"time"
"github.com/apache/iceberg-go"
"github.com/apache/iceberg-go/table"
"github.com/parquet-go/parquet-go"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/seaweedfs/seaweedfs/weed/pb/plugin_pb"
)
func TestParseConfig(t *testing.T) {
@@ -572,14 +577,19 @@ type testEntrySpec struct {
path string
size int64
partition map[int]any
specID int32 // partition spec ID; 0 uses UnpartitionedSpec
}
// makeTestEntries creates manifest entries using UnpartitionedSpec (spec ID 0).
// The specID field in testEntrySpec is ignored here; for multi-spec testing,
// use makeTestEntriesWithSpec instead.
func makeTestEntries(t *testing.T, specs []testEntrySpec) []iceberg.ManifestEntry {
t.Helper()
entries := make([]iceberg.ManifestEntry, 0, len(specs))
for _, spec := range specs {
partSpec := *iceberg.UnpartitionedSpec
dfBuilder, err := iceberg.NewDataFileBuilder(
*iceberg.UnpartitionedSpec,
partSpec,
iceberg.EntryContentData,
spec.path,
iceberg.ParquetFile,
@@ -598,6 +608,471 @@ func makeTestEntries(t *testing.T, specs []testEntrySpec) []iceberg.ManifestEntr
return entries
}
// makeTestEntriesWithSpec creates manifest entries using specific partition specs.
// Each spec in the specs slice can specify a specID; the entry is built using
// a PartitionSpec with that ID.
func makeTestEntriesWithSpec(t *testing.T, specs []testEntrySpec, partSpecs map[int32]iceberg.PartitionSpec) []iceberg.ManifestEntry {
t.Helper()
entries := make([]iceberg.ManifestEntry, 0, len(specs))
for _, s := range specs {
ps, ok := partSpecs[s.specID]
if !ok {
t.Fatalf("spec ID %d not found in partSpecs map", s.specID)
}
dfBuilder, err := iceberg.NewDataFileBuilder(
ps,
iceberg.EntryContentData,
s.path,
iceberg.ParquetFile,
s.partition,
nil, nil,
1,
s.size,
)
if err != nil {
t.Fatalf("failed to build data file %s: %v", s.path, err)
}
snapID := int64(1)
entry := iceberg.NewManifestEntry(iceberg.EntryStatusADDED, &snapID, nil, nil, dfBuilder.Build())
entries = append(entries, entry)
}
return entries
}
func TestBuildCompactionBinsMultipleSpecs(t *testing.T) {
targetSize := int64(256 * 1024 * 1024)
minFiles := 2
// Two partition specs with different IDs
spec0 := iceberg.NewPartitionSpecID(0)
spec1 := iceberg.NewPartitionSpecID(1)
partSpecs := map[int32]iceberg.PartitionSpec{
0: spec0,
1: spec1,
}
// Entries from two different specs with same partition values should go
// to separate bins.
entries := makeTestEntriesWithSpec(t, []testEntrySpec{
{path: "data/s0-f1.parquet", size: 1024, partition: map[int]any{}, specID: 0},
{path: "data/s0-f2.parquet", size: 2048, partition: map[int]any{}, specID: 0},
{path: "data/s1-f1.parquet", size: 1024, partition: map[int]any{}, specID: 1},
{path: "data/s1-f2.parquet", size: 2048, partition: map[int]any{}, specID: 1},
}, partSpecs)
bins := buildCompactionBins(entries, targetSize, minFiles)
if len(bins) != 2 {
t.Fatalf("expected 2 bins (one per spec), got %d", len(bins))
}
// Verify each bin has entries from only one spec
specsSeen := make(map[int32]bool)
for _, bin := range bins {
specsSeen[bin.SpecID] = true
for _, entry := range bin.Entries {
if entry.DataFile().SpecID() != bin.SpecID {
t.Errorf("bin specID=%d contains entry with specID=%d", bin.SpecID, entry.DataFile().SpecID())
}
}
}
if !specsSeen[0] || !specsSeen[1] {
t.Errorf("expected bins for spec 0 and 1, got specs %v", specsSeen)
}
}
func TestBuildCompactionBinsSingleSpec(t *testing.T) {
// Verify existing behavior: all entries with spec 0 still group correctly.
targetSize := int64(256 * 1024 * 1024)
minFiles := 2
spec0 := iceberg.NewPartitionSpecID(0)
partSpecs := map[int32]iceberg.PartitionSpec{0: spec0}
entries := makeTestEntriesWithSpec(t, []testEntrySpec{
{path: "data/f1.parquet", size: 1024, partition: map[int]any{}, specID: 0},
{path: "data/f2.parquet", size: 2048, partition: map[int]any{}, specID: 0},
{path: "data/f3.parquet", size: 4096, partition: map[int]any{}, specID: 0},
}, partSpecs)
bins := buildCompactionBins(entries, targetSize, minFiles)
if len(bins) != 1 {
t.Fatalf("expected 1 bin, got %d", len(bins))
}
if len(bins[0].Entries) != 3 {
t.Errorf("expected 3 entries in bin, got %d", len(bins[0].Entries))
}
if bins[0].SpecID != 0 {
t.Errorf("expected specID=0, got %d", bins[0].SpecID)
}
}
func TestParseConfigApplyDeletes(t *testing.T) {
// Default: true
config := ParseConfig(nil)
if !config.ApplyDeletes {
t.Error("expected ApplyDeletes=true by default")
}
// Explicit false
config = ParseConfig(map[string]*plugin_pb.ConfigValue{
"apply_deletes": {Kind: &plugin_pb.ConfigValue_BoolValue{BoolValue: false}},
})
if config.ApplyDeletes {
t.Error("expected ApplyDeletes=false when explicitly set")
}
// String "false"
config = ParseConfig(map[string]*plugin_pb.ConfigValue{
"apply_deletes": {Kind: &plugin_pb.ConfigValue_StringValue{StringValue: "false"}},
})
if config.ApplyDeletes {
t.Error("expected ApplyDeletes=false when set via string 'false'")
}
}
func TestCollectPositionDeletes(t *testing.T) {
fs, client := startFakeFiler(t)
// Create a position delete Parquet file with file_path + pos columns
type posDeleteRow struct {
FilePath string `parquet:"file_path"`
Pos int64 `parquet:"pos"`
}
var buf bytes.Buffer
writer := parquet.NewWriter(&buf, parquet.SchemaOf(new(posDeleteRow)))
rows := []posDeleteRow{
{"data/file1.parquet", 0},
{"data/file1.parquet", 5},
{"data/file1.parquet", 10},
{"data/file2.parquet", 3},
}
for _, r := range rows {
if err := writer.Write(&r); err != nil {
t.Fatalf("write pos delete row: %v", err)
}
}
if err := writer.Close(); err != nil {
t.Fatalf("close pos delete writer: %v", err)
}
// Store in fake filer
dataDir := "/buckets/test-bucket/ns/tbl/data"
fs.putEntry(dataDir, "pos-delete-1.parquet", &filer_pb.Entry{
Name: "pos-delete-1.parquet",
Content: buf.Bytes(),
})
// Create a manifest entry for the position delete file
spec := *iceberg.UnpartitionedSpec
dfBuilder, err := iceberg.NewDataFileBuilder(
spec,
iceberg.EntryContentPosDeletes,
"data/pos-delete-1.parquet",
iceberg.ParquetFile,
map[int]any{},
nil, nil,
int64(len(rows)),
int64(buf.Len()),
)
if err != nil {
t.Fatalf("build pos delete data file: %v", err)
}
snapID := int64(1)
entry := iceberg.NewManifestEntry(iceberg.EntryStatusADDED, &snapID, nil, nil, dfBuilder.Build())
result, err := collectPositionDeletes(context.Background(), client, "test-bucket", "ns/tbl", []iceberg.ManifestEntry{entry})
if err != nil {
t.Fatalf("collectPositionDeletes: %v", err)
}
// Verify results
if len(result["data/file1.parquet"]) != 3 {
t.Errorf("expected 3 positions for file1, got %d", len(result["data/file1.parquet"]))
}
if len(result["data/file2.parquet"]) != 1 {
t.Errorf("expected 1 position for file2, got %d", len(result["data/file2.parquet"]))
}
// Verify sorted
positions := result["data/file1.parquet"]
for i := 1; i < len(positions); i++ {
if positions[i] <= positions[i-1] {
t.Errorf("positions not sorted: %v", positions)
break
}
}
}
func TestCollectEqualityDeletes(t *testing.T) {
fs, client := startFakeFiler(t)
// Create an equality delete Parquet file with id + name columns
type eqDeleteRow struct {
ID int64 `parquet:"id"`
Name string `parquet:"name"`
}
var buf bytes.Buffer
writer := parquet.NewWriter(&buf, parquet.SchemaOf(new(eqDeleteRow)))
deleteRows := []eqDeleteRow{
{1, "alice"},
{2, "bob"},
{3, "charlie"},
}
for _, r := range deleteRows {
if err := writer.Write(&r); err != nil {
t.Fatalf("write eq delete row: %v", err)
}
}
if err := writer.Close(); err != nil {
t.Fatalf("close eq delete writer: %v", err)
}
dataDir := "/buckets/test-bucket/ns/tbl/data"
fs.putEntry(dataDir, "eq-delete-1.parquet", &filer_pb.Entry{
Name: "eq-delete-1.parquet",
Content: buf.Bytes(),
})
// Create manifest entry with equality field IDs
schema := newTestSchema() // has field 1=id, 2=name
spec := *iceberg.UnpartitionedSpec
dfBuilder, err := iceberg.NewDataFileBuilder(
spec,
iceberg.EntryContentEqDeletes,
"data/eq-delete-1.parquet",
iceberg.ParquetFile,
map[int]any{},
nil, nil,
int64(len(deleteRows)),
int64(buf.Len()),
)
if err != nil {
t.Fatalf("build eq delete data file: %v", err)
}
dfBuilder.EqualityFieldIDs([]int{1, 2}) // id + name
snapID := int64(1)
entry := iceberg.NewManifestEntry(iceberg.EntryStatusADDED, &snapID, nil, nil, dfBuilder.Build())
groups, err := collectEqualityDeletes(context.Background(), client, "test-bucket", "ns/tbl", []iceberg.ManifestEntry{entry}, schema)
if err != nil {
t.Fatalf("collectEqualityDeletes: %v", err)
}
if len(groups) != 1 {
t.Fatalf("expected 1 equality delete group, got %d", len(groups))
}
if len(groups[0].Keys) != 3 {
t.Errorf("expected 3 equality delete keys, got %d", len(groups[0].Keys))
}
if len(groups[0].FieldIDs) != 2 {
t.Errorf("expected 2 field IDs, got %d", len(groups[0].FieldIDs))
}
}
func TestMergeParquetFilesWithPositionDeletes(t *testing.T) {
fs, client := startFakeFiler(t)
// Create two data files with known rows
type dataRow struct {
ID int64 `parquet:"id"`
Name string `parquet:"name"`
}
writeDataFile := func(name string, rows []dataRow) {
var buf bytes.Buffer
w := parquet.NewWriter(&buf, parquet.SchemaOf(new(dataRow)))
for _, r := range rows {
if err := w.Write(&r); err != nil {
t.Fatalf("write row: %v", err)
}
}
if err := w.Close(); err != nil {
t.Fatalf("close: %v", err)
}
dataDir := "/buckets/test-bucket/ns/tbl/data"
fs.putEntry(dataDir, name, &filer_pb.Entry{
Name: name,
Content: buf.Bytes(),
})
}
writeDataFile("file1.parquet", []dataRow{
{1, "alice"}, {2, "bob"}, {3, "charlie"}, {4, "dave"}, {5, "eve"},
})
writeDataFile("file2.parquet", []dataRow{
{6, "frank"}, {7, "grace"},
})
spec := *iceberg.UnpartitionedSpec
makeEntry := func(path string, size int64) iceberg.ManifestEntry {
dfb, err := iceberg.NewDataFileBuilder(spec, iceberg.EntryContentData, path, iceberg.ParquetFile, map[int]any{}, nil, nil, 1, size)
if err != nil {
t.Fatalf("build: %v", err)
}
snapID := int64(1)
return iceberg.NewManifestEntry(iceberg.EntryStatusADDED, &snapID, nil, nil, dfb.Build())
}
entries := []iceberg.ManifestEntry{
makeEntry("data/file1.parquet", 1024),
makeEntry("data/file2.parquet", 512),
}
// Delete rows 1 (bob) and 3 (dave) from file1
posDeletes := map[string][]int64{
"data/file1.parquet": {1, 3},
}
merged, count, err := mergeParquetFiles(
context.Background(), client, "test-bucket", "ns/tbl",
entries, posDeletes, nil, nil,
)
if err != nil {
t.Fatalf("mergeParquetFiles: %v", err)
}
// Original: 5 + 2 = 7 rows, deleted 2 = 5 rows
if count != 5 {
t.Errorf("expected 5 rows after position deletes, got %d", count)
}
// Verify merged output is valid Parquet
reader := parquet.NewReader(bytes.NewReader(merged))
defer reader.Close()
var outputRows []dataRow
for {
var r dataRow
err := reader.Read(&r)
if err == io.EOF {
break
}
if err != nil {
t.Fatalf("read merged row: %v", err)
}
outputRows = append(outputRows, r)
}
if len(outputRows) != 5 {
t.Fatalf("expected 5 output rows, got %d", len(outputRows))
}
// Verify bob (id=2) and dave (id=4) are NOT in output
for _, r := range outputRows {
if r.ID == 2 || r.ID == 4 {
t.Errorf("row with id=%d should have been deleted", r.ID)
}
}
}
func TestMergeParquetFilesWithEqualityDeletes(t *testing.T) {
fs, client := startFakeFiler(t)
type dataRow struct {
ID int64 `parquet:"id"`
Name string `parquet:"name"`
}
var buf bytes.Buffer
w := parquet.NewWriter(&buf, parquet.SchemaOf(new(dataRow)))
dataRows := []dataRow{
{1, "alice"}, {2, "bob"}, {3, "charlie"}, {4, "dave"},
}
for _, r := range dataRows {
if err := w.Write(&r); err != nil {
t.Fatalf("write: %v", err)
}
}
if err := w.Close(); err != nil {
t.Fatalf("close: %v", err)
}
dataDir := "/buckets/test-bucket/ns/tbl/data"
fs.putEntry(dataDir, "data1.parquet", &filer_pb.Entry{
Name: "data1.parquet",
Content: buf.Bytes(),
})
spec := *iceberg.UnpartitionedSpec
dfb, err := iceberg.NewDataFileBuilder(spec, iceberg.EntryContentData, "data/data1.parquet", iceberg.ParquetFile, map[int]any{}, nil, nil, 4, int64(buf.Len()))
if err != nil {
t.Fatalf("build: %v", err)
}
snapID := int64(1)
entries := []iceberg.ManifestEntry{
iceberg.NewManifestEntry(iceberg.EntryStatusADDED, &snapID, nil, nil, dfb.Build()),
}
// Build equality deletes via the collect pipeline so keys match the
// encoding used by mergeParquetFiles.
schema := newTestSchema() // field 1=id, field 2=name
type eqDeleteRow struct {
ID int64 `parquet:"id"`
Name string `parquet:"name"`
}
var eqBuf bytes.Buffer
eqWriter := parquet.NewWriter(&eqBuf, parquet.SchemaOf(new(eqDeleteRow)))
for _, r := range []eqDeleteRow{{0, "bob"}, {0, "dave"}} {
if err := eqWriter.Write(&r); err != nil {
t.Fatalf("write eq delete: %v", err)
}
}
if err := eqWriter.Close(); err != nil {
t.Fatalf("close eq writer: %v", err)
}
eqDataDir := "/buckets/test-bucket/ns/tbl/data"
fs.putEntry(eqDataDir, "eq-del.parquet", &filer_pb.Entry{
Name: "eq-del.parquet", Content: eqBuf.Bytes(),
})
eqDfb, err := iceberg.NewDataFileBuilder(spec, iceberg.EntryContentEqDeletes,
"data/eq-del.parquet", iceberg.ParquetFile, map[int]any{}, nil, nil, 2, int64(eqBuf.Len()))
if err != nil {
t.Fatalf("build eq delete: %v", err)
}
eqDfb.EqualityFieldIDs([]int{2}) // field 2 = name
eqEntry := iceberg.NewManifestEntry(iceberg.EntryStatusADDED, &snapID, nil, nil, eqDfb.Build())
eqGroups, err := collectEqualityDeletes(context.Background(), client, "test-bucket", "ns/tbl",
[]iceberg.ManifestEntry{eqEntry}, schema)
if err != nil {
t.Fatalf("collectEqualityDeletes: %v", err)
}
merged, count, err := mergeParquetFiles(
context.Background(), client, "test-bucket", "ns/tbl",
entries, nil, eqGroups, schema,
)
if err != nil {
t.Fatalf("mergeParquetFiles: %v", err)
}
if count != 2 {
t.Errorf("expected 2 rows after equality deletes, got %d", count)
}
reader := parquet.NewReader(bytes.NewReader(merged))
defer reader.Close()
var outputRows []dataRow
for {
var r dataRow
err := reader.Read(&r)
if err == io.EOF {
break
}
if err != nil {
t.Fatalf("read: %v", err)
}
outputRows = append(outputRows, r)
}
for _, r := range outputRows {
if r.Name == "bob" || r.Name == "dave" {
t.Errorf("row %q should have been deleted", r.Name)
}
}
}
func TestDetectNilRequest(t *testing.T) {
handler := NewHandler(nil)
err := handler.Detect(nil, nil, nil)