From 17f8f54291b23871fafb52519ef5f78d40cdbacb Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Mon, 3 Aug 2026 16:35:24 +0100 Subject: [PATCH 01/12] iceberg: add throughput and profiling bench harnesses An append-mode throughput bench for the Databricks e2e harness (flag- gated) that sweeps records-per-commit and measures sustained egress and per-commit latency against a live Unity Catalog, and local profiling configs plus a wide-schema shredder micro-benchmark for the per-record CPU work. Measurement tooling for the iceberg sink performance effort; carries no production changes. --- .../impl/iceberg/bench/profile_config.yaml | 89 ++++++ .../iceberg/bench/profile_config_schema.yaml | 115 +++++++ .../iceberg/bench/shred_wide_bench_test.go | 142 +++++++++ .../e2e/databricks/throughput_bench_test.go | 295 ++++++++++++++++++ 4 files changed, 641 insertions(+) create mode 100644 internal/impl/iceberg/bench/profile_config.yaml create mode 100644 internal/impl/iceberg/bench/profile_config_schema.yaml create mode 100644 internal/impl/iceberg/bench/shred_wide_bench_test.go create mode 100644 internal/impl/iceberg/e2e/databricks/throughput_bench_test.go diff --git a/internal/impl/iceberg/bench/profile_config.yaml b/internal/impl/iceberg/bench/profile_config.yaml new file mode 100644 index 0000000000..0d990ac177 --- /dev/null +++ b/internal/impl/iceberg/bench/profile_config.yaml @@ -0,0 +1,89 @@ +# Profiling variant of benchmark_config.yaml, for attributing per-record CPU +# cost in the iceberg sink. +# +# Differences from benchmark_config.yaml: +# - ~1.2KB high-entropy JSON payload (uuid-heavy) instead of the ~150B event +# - the payload is serialised to raw bytes in the pipeline (root = content()) +# so the iceberg output performs a real JSON parse per record, matching the +# production Kafka -> iceberg path. Messages produced by `generate` are +# otherwise already structured and AsStructured() would be free. +# - batching.count 10000 so commits are amortised and per-record CPU dominates +# +# pprof endpoints are exposed at http://localhost:4195/debug/pprof/ via +# http.debug_endpoints. Capture with: +# curl -o cpu.pb.gz 'http://localhost:4195/debug/pprof/profile?seconds=60' +# curl -o allocs.pb.gz 'http://localhost:4195/debug/pprof/allocs' +# +# Run at 1 core with GOMAXPROCS=1 (taskset does not exist on darwin). + +http: + debug_endpoints: true + +input: + generate: + count: ${COUNT:0} + interval: "" + mapping: | + root.id = counter() + root.user_id = (counter() % 10000) + 1 + root.session_id = uuid_v4() + root.trace_id = uuid_v4() + root.span_id = uuid_v4() + root.request_id = uuid_v4() + root.device_id = uuid_v4() + root.correlation_id = uuid_v4() + root.event_type = ["click", "view", "purchase", "scroll", "hover"].index(counter() % 5) + root.country = ["US", "GB", "DE", "FR", "JP", "BR", "IN", "AU"].index(counter() % 8) + root.value = random_int(max: 1000000) + root.amount = random_int(max: 10000000) / 100.0 + root.score = random_int(max: 100000) / 1000.0 + root.latency_ms = random_int(max: 30000) + root.is_mobile = counter() % 2 == 0 + root.user_agent = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36" + root.url = "https://shop.example.com/products/" + uuid_v4() + "?ref=" + uuid_v4() + root.referrer = "https://www.google.com/search?q=" + uuid_v4() + root.payload_a = uuid_v4() + ":" + uuid_v4() + root.payload_b = uuid_v4() + ":" + uuid_v4() + root.payload_c = uuid_v4() + ":" + uuid_v4() + root.payload_d = uuid_v4() + ":" + uuid_v4() + root.description = "synthetic high entropy event record number " + counter().string() + " for per-record cpu profiling" + root.ts = now() + +pipeline: + processors: + # Force the message down to raw bytes so the iceberg output pays the JSON + # decode cost (AsStructured), as it does when reading from Kafka. + - mapping: 'root = content()' + - benchmark: + interval: 1s + count_bytes: true + +output: + iceberg: + catalog: + url: "${CATALOG_URL:http://localhost:8181}" + namespace: bench + table: "${TABLE:events_profile}" + max_in_flight: ${MIF:4} + storage: + aws_s3: + bucket: "${MINIO_BUCKET:warehouse}" + region: "${MINIO_REGION:us-east-1}" + endpoint: "${MINIO_ENDPOINT:http://localhost:9000}" + force_path_style_urls: true + credentials: + id: "${MINIO_USER:admin}" + secret: "${MINIO_PASSWORD:password}" + schema_evolution: + enabled: true + batching: + count: 10000 + period: 5s + +logger: + level: INFO + +metrics: + prometheus: + add_process_metrics: true + add_go_metrics: true diff --git a/internal/impl/iceberg/bench/profile_config_schema.yaml b/internal/impl/iceberg/bench/profile_config_schema.yaml new file mode 100644 index 0000000000..b44b639405 --- /dev/null +++ b/internal/impl/iceberg/bench/profile_config_schema.yaml @@ -0,0 +1,115 @@ +# Declared-schema variant of profile_config.yaml, for measuring whether a +# declared schema reduces per-record CPU cost. +# +# Identical workload, but every message carries schema metadata (the common +# schema format shared with parquet_encode's schema_metadata) in the +# `iceberg_schema` metadata field, and the output is configured with +# schema_evolution.schema_metadata: iceberg_schema. This exercises the +# declared-schema path: type resolution at table creation/evolution comes from +# the declared schema, and the shredder receives field schema metadata +# (SetFieldSchemaMetadata) instead of relying purely on dynamic inference. +# +# Declared types mirror what inference produces for the same payload so the +# resulting table is identical to the schemaless run (apples-to-apples). +# +# Run at 1 core with GOMAXPROCS=1. See profile_config.yaml for capture notes. + +http: + debug_endpoints: true + +input: + generate: + count: ${COUNT:0} + interval: "" + mapping: | + root.id = counter() + root.user_id = (counter() % 10000) + 1 + root.session_id = uuid_v4() + root.trace_id = uuid_v4() + root.span_id = uuid_v4() + root.request_id = uuid_v4() + root.device_id = uuid_v4() + root.correlation_id = uuid_v4() + root.event_type = ["click", "view", "purchase", "scroll", "hover"].index(counter() % 5) + root.country = ["US", "GB", "DE", "FR", "JP", "BR", "IN", "AU"].index(counter() % 8) + root.value = random_int(max: 1000000) + root.amount = random_int(max: 10000000) / 100.0 + root.score = random_int(max: 100000) / 1000.0 + root.latency_ms = random_int(max: 30000) + root.is_mobile = counter() % 2 == 0 + root.user_agent = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36" + root.url = "https://shop.example.com/products/" + uuid_v4() + "?ref=" + uuid_v4() + root.referrer = "https://www.google.com/search?q=" + uuid_v4() + root.payload_a = uuid_v4() + ":" + uuid_v4() + root.payload_b = uuid_v4() + ":" + uuid_v4() + root.payload_c = uuid_v4() + ":" + uuid_v4() + root.payload_d = uuid_v4() + ":" + uuid_v4() + root.description = "synthetic high entropy event record number " + counter().string() + " for per-record cpu profiling" + root.ts = now() + meta iceberg_schema = { + "type": "OBJECT", + "children": [ + {"name": "id", "type": "INT64"}, + {"name": "user_id", "type": "INT64"}, + {"name": "session_id", "type": "STRING"}, + {"name": "trace_id", "type": "STRING"}, + {"name": "span_id", "type": "STRING"}, + {"name": "request_id", "type": "STRING"}, + {"name": "device_id", "type": "STRING"}, + {"name": "correlation_id", "type": "STRING"}, + {"name": "event_type", "type": "STRING"}, + {"name": "country", "type": "STRING"}, + {"name": "value", "type": "INT64"}, + {"name": "amount", "type": "FLOAT64"}, + {"name": "score", "type": "FLOAT64"}, + {"name": "latency_ms", "type": "INT64"}, + {"name": "is_mobile", "type": "BOOLEAN"}, + {"name": "user_agent", "type": "STRING"}, + {"name": "url", "type": "STRING"}, + {"name": "referrer", "type": "STRING"}, + {"name": "payload_a", "type": "STRING"}, + {"name": "payload_b", "type": "STRING"}, + {"name": "payload_c", "type": "STRING"}, + {"name": "payload_d", "type": "STRING"}, + {"name": "description", "type": "STRING"}, + {"name": "ts", "type": "STRING"} + ] + } + +pipeline: + processors: + - mapping: 'root = content()' + - benchmark: + interval: 1s + count_bytes: true + +output: + iceberg: + catalog: + url: "${CATALOG_URL:http://localhost:8181}" + namespace: bench + table: "${TABLE:events_profile_schema}" + max_in_flight: ${MIF:4} + storage: + aws_s3: + bucket: "${MINIO_BUCKET:warehouse}" + region: "${MINIO_REGION:us-east-1}" + endpoint: "${MINIO_ENDPOINT:http://localhost:9000}" + force_path_style_urls: true + credentials: + id: "${MINIO_USER:admin}" + secret: "${MINIO_PASSWORD:password}" + schema_evolution: + enabled: true + schema_metadata: iceberg_schema + batching: + count: 10000 + period: 5s + +logger: + level: INFO + +metrics: + prometheus: + add_process_metrics: true + add_go_metrics: true diff --git a/internal/impl/iceberg/bench/shred_wide_bench_test.go b/internal/impl/iceberg/bench/shred_wide_bench_test.go new file mode 100644 index 0000000000..b7908de284 --- /dev/null +++ b/internal/impl/iceberg/bench/shred_wide_bench_test.go @@ -0,0 +1,142 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/redpanda/blob/master/licenses/rcl.md + +// Package bench holds micro-benchmarks that mirror the end-to-end profiling +// workload (profile_config.yaml) so per-record costs can be attributed +// without standing up infrastructure. Part of the iceberg sink per-record CPU +// profiling effort. +package bench + +import ( + "fmt" + "testing" + + "github.com/apache/iceberg-go" + + "github.com/redpanda-data/benthos/v4/public/schema" + + "github.com/redpanda-data/connect/v4/internal/impl/iceberg/icebergx" + "github.com/redpanda-data/connect/v4/internal/impl/iceberg/shredder" +) + +// discardSink mirrors the shredder package's benchmark sink: no work, no +// allocation, so the benchmark isolates the shredder's own per-record cost. +type discardSink struct{} + +func (discardSink) EmitValue(shredder.ShreddedValue) error { return nil } +func (discardSink) OnNewField(icebergx.Path, string, any) {} + +// wideSchema mirrors the 24-column table created by profile_config.yaml +// (~1.2KB high-entropy JSON events). +func wideSchema() *iceberg.Schema { + names := wideFieldNames() + fields := make([]iceberg.NestedField, 0, len(names)) + for i, n := range names { + var typ iceberg.Type + switch n { + case "id", "user_id", "value", "latency_ms": + typ = iceberg.PrimitiveTypes.Int64 + case "amount", "score": + typ = iceberg.PrimitiveTypes.Float64 + case "is_mobile": + typ = iceberg.PrimitiveTypes.Bool + default: + typ = iceberg.PrimitiveTypes.String + } + fields = append(fields, iceberg.NestedField{ID: i + 1, Name: n, Type: typ}) + } + return iceberg.NewSchema(1, fields...) +} + +func wideFieldNames() []string { + return []string{ + "id", "user_id", "session_id", "trace_id", "span_id", "request_id", + "device_id", "correlation_id", "event_type", "country", "value", + "amount", "score", "latency_ms", "is_mobile", "user_agent", "url", + "referrer", "payload_a", "payload_b", "payload_c", "payload_d", + "description", "ts", + } +} + +func wideRecord() map[string]any { + return map[string]any{ + "id": int64(123456), + "user_id": int64(4212), + "session_id": "0d9c9c3e-9df6-4c4f-8a91-2e6f1a9f7f10", + "trace_id": "5c1a67aa-30e7-4a83-9b0e-fd3f9a3f6c1b", + "span_id": "e3b7c5d2-8f14-4a6e-b291-7c8e9d0f1a2b", + "request_id": "9a8b7c6d-5e4f-4a3b-8c2d-1e0f9a8b7c6d", + "device_id": "1f2e3d4c-5b6a-4978-8695-a4b3c2d1e0f9", + "correlation_id": "abcdef01-2345-4678-9abc-def012345678", + "event_type": "purchase", + "country": "GB", + "value": int64(778123), + "amount": 42421.42, + "score": 73.113, + "latency_ms": int64(2231), + "is_mobile": false, + "user_agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36", + "url": "https://shop.example.com/products/0d9c9c3e-9df6-4c4f-8a91-2e6f1a9f7f10?ref=5c1a67aa-30e7-4a83-9b0e-fd3f9a3f6c1b", + "referrer": "https://www.google.com/search?q=e3b7c5d2-8f14-4a6e-b291-7c8e9d0f1a2b", + "payload_a": "9a8b7c6d-5e4f-4a3b-8c2d-1e0f9a8b7c6d:1f2e3d4c-5b6a-4978-8695-a4b3c2d1e0f9", + "payload_b": "0d9c9c3e-9df6-4c4f-8a91-2e6f1a9f7f10:5c1a67aa-30e7-4a83-9b0e-fd3f9a3f6c1b", + "payload_c": "e3b7c5d2-8f14-4a6e-b291-7c8e9d0f1a2b:9a8b7c6d-5e4f-4a3b-8c2d-1e0f9a8b7c6d", + "payload_d": "abcdef01-2345-4678-9abc-def012345678:1f2e3d4c-5b6a-4978-8695-a4b3c2d1e0f9", + "description": "synthetic high entropy event record number 123456 for per-record cpu profiling", + "ts": "2026-08-03T16:10:00.000000000+01:00", + } +} + +// wideFieldCommons builds the per-field schema metadata that +// writer.messagesToParquet installs on the shredder when the output's +// schema_evolution.schema_metadata is configured, declaring the same types +// inference would produce. +func wideFieldCommons(s *iceberg.Schema) map[int]*schema.Common { + byID := make(map[int]*schema.Common) + for _, f := range s.Fields() { + var t schema.CommonType + switch f.Type { + case iceberg.PrimitiveTypes.Int64: + t = schema.Int64 + case iceberg.PrimitiveTypes.Float64: + t = schema.Float64 + case iceberg.PrimitiveTypes.Bool: + t = schema.Boolean + default: + t = schema.String + } + byID[f.ID] = &schema.Common{Name: f.Name, Type: t, Optional: true} + } + return byID +} + +// BenchmarkShredWide measures per-record shred cost for the 24-column +// profiling payload, with and without declared field schema metadata +// (the shredder-side effect of the output's declared-schema path). Run: +// +// GOMAXPROCS=1 go test -bench BenchmarkShredWide -benchmem -run '^$' ./internal/impl/iceberg/bench/ +func BenchmarkShredWide(b *testing.B) { + for _, declared := range []bool{false, true} { + b.Run(fmt.Sprintf("declared_schema=%v", declared), func(b *testing.B) { + rs := shredder.NewRecordShredder(wideSchema(), true) + if declared { + rs.SetFieldSchemaMetadata(wideFieldCommons(wideSchema())) + } + record := wideRecord() + sink := discardSink{} + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := rs.Shred(record, sink); err != nil { + b.Fatal(err) + } + } + }) + } +} diff --git a/internal/impl/iceberg/e2e/databricks/throughput_bench_test.go b/internal/impl/iceberg/e2e/databricks/throughput_bench_test.go new file mode 100644 index 0000000000..4cdeaa435f --- /dev/null +++ b/internal/impl/iceberg/e2e/databricks/throughput_bench_test.go @@ -0,0 +1,295 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/connect/blob/main/licenses/rcl.md + +package databrickse2e + +import ( + "bytes" + "flag" + "fmt" + "log/slog" + "math/rand" + "slices" + "strings" + "sync" + "testing" + "time" + + "github.com/apache/iceberg-go" + "github.com/stretchr/testify/require" + + "github.com/redpanda-data/benthos/v4/public/service" + + icebergimpl "github.com/redpanda-data/connect/v4/internal/impl/iceberg" +) + +// TestDatabricksThroughput characterizes the "small-commit +// throughput trap" against a live Unity Catalog: plain APPEND commits (zero +// RowOpConfig — NOT copy-on-write) at a sweep of records-per-Route batch +// sizes, each driven for a fixed wall window. Because the router commits +// synchronously per Route call, per-call latency IS per-commit latency, and +// sustained throughput is records/window. The smallest point (300) exposes +// UC's floor commit latency for pure appends, comparable against AWS Glue's +// ~320ms and the COW bench's ~7-10s overwrites. +// +// Gated behind -databricks.throughput because it holds a live catalog busy +// for ~15 minutes of real time. + +var dbxThroughput = flag.Bool("databricks.throughput", false, "run the append throughput bench (drives the live catalog for ~15 minutes)") + +const ( + // throughputWindow is the wall-clock measurement window per batch-size + // point. The last Route call may overshoot it; throughput uses the actual + // elapsed time at that call's completion. + throughputWindow = 3 * time.Minute + // bigRouteCutoff: if the first measured Route call of a point exceeds + // this, the point is truncated to two calls total instead of the full + // window (per the run plan for the largest batch point). + bigRouteCutoff = 90 * time.Second + // payloadLen makes each JSON record ~1.2KB, mirroring the earlier + // benchmark methodology (id + seq + ~1.1KB string payload + JSON framing). + payloadLen = 1100 +) + +// syncBuffer is a concurrency-safe bytes.Buffer for the capturing logger — +// the committer may log from goroutines. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// newCapturingRouter mirrors newRouter but wires a capturing slog logger (the +// output_iceberg_test.go seam) so committer warnings — commit retries, +// unknown-state, prohibited-key stripping — are collectable evidence. +func newCapturingRouter(t *testing.T, namespace, tableName string, rowOp icebergimpl.RowOpConfig) (*icebergimpl.Router, *syncBuffer) { + t.Helper() + namespaceStr, err := service.NewInterpolatedString(namespace) + require.NoError(t, err) + tableStr, err := service.NewInterpolatedString(tableName) + require.NoError(t, err) + + sb := &syncBuffer{} + logger := service.NewLoggerFromSlog(slog.New(slog.NewTextHandler(sb, &slog.HandlerOptions{ + Level: slog.LevelWarn, + }))) + + commitCfg := icebergimpl.CommitConfig{ + ManifestMergeEnabled: true, + MaxSnapshotAge: 24 * time.Hour, + MaxRetries: 3, + } + router := icebergimpl.NewRouter(buildCatalogConfig(), namespaceStr, tableStr, true, + icebergimpl.SchemaEvolutionConfig{Enabled: true}, commitCfg, rowOp, nil, logger) + t.Cleanup(func() { router.Close() }) + return router, sb +} + +// benchBatch builds a batch of structured append messages (~1.2KB of JSON +// each). Payload strings are zero-copy slices of a shared random pool so +// generation cost stays negligible next to multi-second commits; startID +// advances per call so ids stay unique and content varies across commits. +func benchBatch(pool string, startID int64, size int) service.MessageBatch { + rng := rand.New(rand.NewSource(startID)) //nolint:gosec // bench entropy, not crypto + msgs := make(service.MessageBatch, size) + for i := range msgs { + off := rng.Intn(len(pool) - payloadLen) + m := service.NewMessage(nil) + m.SetStructured(map[string]any{ + "id": startID + int64(i), + "seq": int64(i), + "payload": pool[off : off+payloadLen], + }) + msgs[i] = m + } + return msgs +} + +// warnEvidence tallies committer warning lines relevant to the parked #4591 +// throttle/5xx hypothesis and keeps a few (redacted) samples. +type warnEvidence struct { + commitRetries int + prohibitedKeys int + unknownState int + otherWarnings int + samples []string +} + +func collectWarnings(logged string) warnEvidence { + var ev warnEvidence + for line := range strings.SplitSeq(logged, "\n") { + if line == "" { + continue + } + lower := strings.ToLower(line) + switch { + case strings.Contains(line, "Commit attempt"): + ev.commitRetries++ + case strings.Contains(lower, "prohibit"): + ev.prohibitedKeys++ + case strings.Contains(lower, "unknown state"): + ev.unknownState++ + case strings.Contains(lower, "level=warn"), strings.Contains(lower, "level=error"): + ev.otherWarnings++ + default: + continue + } + if len(ev.samples) < 5 { + ev.samples = append(ev.samples, redact(line)) + } + } + return ev +} + +func percentile(sorted []time.Duration, p float64) time.Duration { + if len(sorted) == 0 { + return 0 + } + idx := int(p * float64(len(sorted)-1)) + return sorted[idx] +} + +type throughputPoint struct { + batchSize int + commits int + records int64 + elapsed time.Duration + p50, p95, max time.Duration + failures int + warnings warnEvidence +} + +func (pt throughputPoint) String() string { + perMin := float64(pt.commits) / pt.elapsed.Minutes() + return fmt.Sprintf("batch=%d commits=%d records=%d window=%v rec/s=%.0f commit_p50=%v p95=%v max=%v commits/min=%.1f failures=%d retries=%d prohibited=%d unknown=%d otherWarn=%d", + pt.batchSize, pt.commits, pt.records, pt.elapsed.Round(time.Second), + float64(pt.records)/pt.elapsed.Seconds(), + pt.p50.Round(time.Millisecond), pt.p95.Round(time.Millisecond), pt.max.Round(time.Millisecond), + perMin, pt.failures, + pt.warnings.commitRetries, pt.warnings.prohibitedKeys, pt.warnings.unknownState, pt.warnings.otherWarnings) +} + +func TestDatabricksThroughput(t *testing.T) { + skipIfNotConfigured(t) + if !*dbxThroughput { + t.Skip("set -databricks.throughput to run the append throughput bench") + } + ctx := t.Context() + + // Shared random pool for payload slicing (allocated once for the run). + poolRng := rand.New(rand.NewSource(42)) //nolint:gosec // bench entropy, not crypto + const alnum = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + poolBytes := make([]byte, 64*1024) + for i := range poolBytes { + poolBytes[i] = alnum[poolRng.Intn(len(alnum))] + } + pool := string(poolBytes) + + var results []throughputPoint + + for _, batchSize := range []int{300, 5000, 50000, 200000} { + t.Run(fmt.Sprintf("batch_%d", batchSize), func(t *testing.T) { + tableName := uniqueTableName(fmt.Sprintf("tput_%d", batchSize)) + t.Cleanup(func() { dropTable(t, tableName) }) + + // Pre-create the table so the measured window contains only + // append commits — no CREATE TABLE inside the measurement. + client := newCatalogClient(t, ctx) + _, err := client.CreateTable(ctx, tableName, iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.Int64Type{}}, + iceberg.NestedField{ID: 2, Name: "seq", Type: iceberg.Int64Type{}}, + iceberg.NestedField{ID: 3, Name: "payload", Type: iceberg.StringType{}}, + )) + require.NoError(t, err) + + // Zero RowOpConfig => every message is a plain append. + router, logBuf := newCapturingRouter(t, *dbxSchema, tableName, icebergimpl.RowOpConfig{}) + + // Uncounted warmup: absorbs the first-write bootstrap (table + // load + timestamp-encoding property stamp) so measured calls + // are steady-state appends. + nextID := int64(1) + require.NoError(t, router.Route(ctx, benchBatch(pool, nextID, 10))) + nextID += 10 + + var ( + latencies []time.Duration + records int64 + failures int + ) + start := time.Now() + var elapsed time.Duration + for { + batch := benchBatch(pool, nextID, batchSize) + nextID += int64(batchSize) + + callStart := time.Now() + routeErr := router.Route(ctx, batch) + callDur := time.Since(callStart) + elapsed = time.Since(start) + + if routeErr != nil { + failures++ + t.Logf("batch=%d commit %d FAILED after %v: %v", batchSize, len(latencies)+failures, callDur.Round(time.Millisecond), redact(routeErr.Error())) + if failures >= 3 { + t.Logf("batch=%d: aborting point after %d consecutive-ish failures", batchSize, failures) + break + } + } else { + latencies = append(latencies, callDur) + records += int64(batchSize) + t.Logf("batch=%d commit %d: %v (%.0f rec/s within call)", batchSize, len(latencies), callDur.Round(time.Millisecond), float64(batchSize)/callDur.Seconds()) + } + + if elapsed >= throughputWindow { + break + } + // Very large batches: cap at two calls if a single call + // blows past the cutoff (keeps live time bounded). + if len(latencies)+failures >= 2 && callDur > bigRouteCutoff { + t.Logf("batch=%d: truncating point to %d calls (single Route exceeded %v)", batchSize, len(latencies)+failures, bigRouteCutoff) + break + } + } + + slices.Sort(latencies) + pt := throughputPoint{ + batchSize: batchSize, + commits: len(latencies), + records: records, + elapsed: elapsed, + p50: percentile(latencies, 0.50), + p95: percentile(latencies, 0.95), + max: percentile(latencies, 1.0), + failures: failures, + warnings: collectWarnings(logBuf.String()), + } + results = append(results, pt) + t.Logf("POINT RESULT: %s", pt) + for _, s := range pt.warnings.samples { + t.Logf(" warning sample: %s", s) + } + }) + } + + t.Log("=== THROUGHPUT SWEEP SUMMARY (append mode, live Unity Catalog) ===") + for _, pt := range results { + t.Logf(" %s", pt) + } +} From 2e80f3e050f0f0a56dff99e6b00700453b327a70 Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Tue, 18 Aug 2026 15:26:11 +0100 Subject: [PATCH 02/12] iceberg: cut two per-record map allocations from the shredder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shredding a record built two maps per struct: an index of input keys by their match-key, and a set of keys that matched a schema field. Both exist to support case-insensitive matching, where several input keys can fold onto one schema field and that ambiguity has to be reported rather than resolved silently. When matching is case-sensitive the match-key is the identity function, so neither map earns its keep: a field's value is just value[field.Name], and case-collisions are impossible because Go map keys are themselves case-sensitive. That is the default (case_sensitive_columns: true), so it is the path almost all traffic takes. Split the two apart. shredStructExact handles case-sensitive matching with direct lookups; the existing body moves to shredStructFolded unchanged. Unknown-field detection now counts how many input keys a schema field claimed and skips the scan entirely when they are all accounted for — the steady state once a schema has settled — building a lookup set only on the rare path where something genuinely is unknown. BenchmarkShredWide at GOMAXPROCS=1 (benchstat, n=8, p=0.000): sec/op 4.369µ -> 1.472µ -66.3% B/op 4.312Ki -> 1.609Ki -62.7% allocs/op 71 -> 41 -42.3% That is the shredder in isolation. Earlier profiling attributed ~27% of the sink's CPU to shredding, so the end-to-end saving should be appreciable but smaller; it has not been measured yet. Because this is purely an optimisation it must not change observable behaviour, so TestShredStructPathsAgree drives the same schema and record through both implementations and requires identical emitted values, new-field notifications and errors across full matches, unknown keys (top-level and nested), missing optionals, explicit nulls, both required-field error paths, and the empty schema and record edges. --- internal/impl/iceberg/shredder/shredder.go | 104 +++++++++++ .../shredder/shredder_paths_agree_test.go | 167 ++++++++++++++++++ 2 files changed, 271 insertions(+) create mode 100644 internal/impl/iceberg/shredder/shredder_paths_agree_test.go diff --git a/internal/impl/iceberg/shredder/shredder.go b/internal/impl/iceberg/shredder/shredder.go index cadc88eca0..2034125572 100644 --- a/internal/impl/iceberg/shredder/shredder.go +++ b/internal/impl/iceberg/shredder/shredder.go @@ -152,6 +152,33 @@ func (rs *RecordShredder) shredStruct( path icebergx.Path, repLevel, defLevel, maxRepLevel int, sink Sink, +) error { + // Case-sensitive matching makes the match-key the identity function, so + // schema fields can be looked up directly in the input map and neither of + // the two per-record maps below is needed. That path is split out because + // this function runs once per record (and once per nested struct within + // it), and a 1-vCPU allocation profile attributed a material share of the + // sink's total allocations to those two maps. + if rs.caseSensitive { + return rs.shredStructExact(fields, value, path, repLevel, defLevel, maxRepLevel, sink) + } + + return rs.shredStructFolded(fields, value, path, repLevel, defLevel, maxRepLevel, sink) +} + +// shredStructFolded is the case-insensitive path: input keys are matched against +// schema field names by their folded (lowercased) form, which means several +// distinct input keys can collide on one schema field and that collision has to +// be reported rather than silently resolved. +// +// shredStructExact must stay behaviourally identical to this for input that +// happens to match exactly; TestShredStructPathsAgree pins that. +func (rs *RecordShredder) shredStructFolded( + fields []iceberg.NestedField, + value map[string]any, + path icebergx.Path, + repLevel, defLevel, maxRepLevel int, + sink Sink, ) error { // Build an index of input keys by their match-key (the original key in // case-sensitive mode, or its lowercase form in case-insensitive mode). @@ -227,6 +254,83 @@ func (rs *RecordShredder) shredStruct( return nil } +// shredStructExact is shredStruct's case-sensitive equivalent: input keys must +// match schema field names byte-for-byte, so a field's value is just +// value[field.Name] and case-collision ambiguity is impossible (Go map keys are +// themselves case-sensitive). +// +// It must stay behaviourally identical to the general path for case-sensitive +// shredders — same required-field errors, same null handling, same +// OnNewField notifications, same traversal order of fields. +func (rs *RecordShredder) shredStructExact( + fields []iceberg.NestedField, + value map[string]any, + path icebergx.Path, + repLevel, defLevel, maxRepLevel int, + sink Sink, +) error { + // Count how many input keys were claimed by a schema field, so unknown-field + // detection below can usually be skipped without tracking a set. + matchedKeys := 0 + + for _, field := range fields { + fieldValue, exists := value[field.Name] + if exists { + matchedKeys++ + } + + // Validate required fields. + if field.Required && (!exists || fieldValue == nil) { + return &RequiredFieldNullError{field, path} + } + + // Compute this field's definition level contribution. + fieldDefLevel := defLevel + if !field.Required { + fieldDefLevel++ // Optional field adds to max def level. + } + + // Build path for this field. The schema's casing is the input's casing + // here, so no canonicalisation is needed. + fieldPath := append(path, icebergx.PathSegment{Kind: icebergx.PathField, Name: field.Name}) + + if !exists || fieldValue == nil { + // Field is null or missing - emit null for all leaf descendants. + if err := rs.shredNull(field.Type, field.ID, repLevel, defLevel, sink); err != nil { + return err + } + continue + } + + if err := rs.shredValue(field.Type, field.ID, fieldValue, fieldPath, repLevel, fieldDefLevel, maxRepLevel, sink); err != nil { + return fmt.Errorf("field %q: %w", field.Name, err) + } + } + + // Detect unknown fields in input. Field names are unique within a struct, + // so each matched field claimed exactly one distinct input key: when the + // counts agree, every key is accounted for and there is nothing to report. + // That is the steady state once a schema has stabilised, and it makes the + // common case allocation-free. (A count above len(value) is impossible for + // a well-formed schema, and would simply fall through to the scan.) + if matchedKeys == len(value) { + return nil + } + + // Something is unknown: pay for a lookup set now, on the rare path only. + known := make(map[string]struct{}, len(fields)) + for _, field := range fields { + known[field.Name] = struct{}{} + } + for key, val := range value { + if _, ok := known[key]; !ok { + sink.OnNewField(slices.Clone(path), key, val) + } + } + + return nil +} + // matchKey returns the lookup key used to compare an input record key against // a schema field name. In case-sensitive mode it is the identity; in // case-insensitive mode it folds to lowercase. diff --git a/internal/impl/iceberg/shredder/shredder_paths_agree_test.go b/internal/impl/iceberg/shredder/shredder_paths_agree_test.go new file mode 100644 index 0000000000..684818db3d --- /dev/null +++ b/internal/impl/iceberg/shredder/shredder_paths_agree_test.go @@ -0,0 +1,167 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/redpanda/blob/master/licenses/rcl.md + +package shredder + +import ( + "fmt" + "sort" + "testing" + + "github.com/apache/iceberg-go" + "github.com/stretchr/testify/require" +) + +// TestShredStructPathsAgree pins the case-sensitive fast path +// (shredStructExact) to the general folded path (shredStructFolded). +// +// shredStructExact exists purely to avoid two per-record map allocations when +// key matching is exact — which is the default (case_sensitive_columns: true) +// and therefore the path almost all traffic takes. It is an optimisation, so it +// must not change observable behaviour by even one emitted value, new-field +// notification, or error. This test drives the same schema and record through +// both implementations and demands identical output. +// +// It covers the cases the optimisation actually reasons about: every key +// matching (the allocation-free steady state), extra unknown keys (the fallback +// scan), missing fields, explicit nulls, required-field violations, and nesting +// — plus an empty record and an empty schema, where the matched-count shortcut +// is most likely to be wrong. +func TestShredStructPathsAgree(t *testing.T) { + nested := iceberg.NestedField{ + ID: 10, + Name: "inner", + Type: &iceberg.StructType{FieldList: []iceberg.NestedField{ + {ID: 11, Name: "a", Type: iceberg.PrimitiveTypes.Int64, Required: false}, + {ID: 12, Name: "b", Type: iceberg.PrimitiveTypes.String, Required: false}, + }}, + Required: false, + } + + schema := iceberg.NewSchema(1, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true}, + iceberg.NestedField{ID: 2, Name: "name", Type: iceberg.PrimitiveTypes.String, Required: false}, + iceberg.NestedField{ID: 3, Name: "flag", Type: iceberg.PrimitiveTypes.Bool, Required: false}, + nested, + ) + + emptySchema := iceberg.NewSchema(2) + + cases := []struct { + name string + schema *iceberg.Schema + record map[string]any + }{ + { + name: "all fields present", + schema: schema, + record: map[string]any{ + "id": int64(1), "name": "a", "flag": true, + "inner": map[string]any{"a": int64(2), "b": "c"}, + }, + }, + { + name: "exact match, no unknowns (allocation-free steady state)", + schema: schema, + record: map[string]any{"id": int64(1), "name": "a", "flag": false, "inner": nil}, + }, + { + name: "one unknown key", + schema: schema, + record: map[string]any{"id": int64(1), "surprise": "x"}, + }, + { + name: "several unknown keys", + schema: schema, + record: map[string]any{"id": int64(1), "x": 1, "y": "two", "z": nil}, + }, + { + name: "unknown key nested inside a known struct", + schema: schema, + record: map[string]any{ + "id": int64(1), + "inner": map[string]any{"a": int64(2), "nope": "surprise"}, + }, + }, + { + name: "missing optional fields", + schema: schema, + record: map[string]any{"id": int64(7)}, + }, + { + name: "explicit nulls", + schema: schema, + record: map[string]any{"id": int64(7), "name": nil, "flag": nil, "inner": nil}, + }, + { + name: "required field missing (error path)", + schema: schema, + record: map[string]any{"name": "no id here"}, + }, + { + name: "required field explicitly null (error path)", + schema: schema, + record: map[string]any{"id": nil}, + }, + { + name: "empty record", + schema: schema, + record: map[string]any{}, + }, + { + name: "empty schema, empty record", + schema: emptySchema, + record: map[string]any{}, + }, + { + name: "empty schema, all keys unknown", + schema: emptySchema, + record: map[string]any{"a": 1, "b": 2}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Both shredders are case-sensitive: the folded path is exercised + // directly so the comparison isolates the implementations rather + // than the matching mode. + rs := NewRecordShredder(tc.schema, true) + fields := tc.schema.Fields() + + exactSink := &testSink{} + exactErr := rs.shredStructExact(fields, tc.record, nil, 0, 0, 0, exactSink) + + foldedSink := &testSink{} + foldedErr := rs.shredStructFolded(fields, tc.record, nil, 0, 0, 0, foldedSink) + + if foldedErr != nil { + require.EqualError(t, exactErr, foldedErr.Error(), + "fast path must fail exactly as the general path does") + return + } + require.NoError(t, exactErr, "fast path errored where the general path did not") + + require.Equal(t, foldedSink.values, exactSink.values, + "emitted values must be identical (including order)") + + // New-field notification order follows Go map iteration, which is + // randomised, so compare as sets. + require.Equal(t, sortedNewFields(foldedSink.newFields), sortedNewFields(exactSink.newFields), + "new-field notifications must be identical") + }) + } +} + +func sortedNewFields(in []newFieldRecord) []string { + out := make([]string, 0, len(in)) + for _, nf := range in { + out = append(out, fmt.Sprintf("path=%v name=%s value=%v", nf.path, nf.name, nf.value)) + } + sort.Strings(out) + return out +} From 42e4bc6271f7cf4d7791b200677a7d943bab3da3 Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Tue, 18 Aug 2026 15:26:24 +0100 Subject: [PATCH 03/12] iceberg: add a commit-regime harness for latency-bound throughput MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sink's headline throughput problem is latency-bound rather than CPU-bound: against a slow catalog, throughput is roughly records-per-commit divided by commit latency. This harness isolates that regime so it can be reasoned about without a hosted catalog. latentCatalog wraps the existing in-memory test catalog with a configurable per-commit delay and a commit counter, and the sweep drives N concurrent submitters against it, reporting records/sec, records/commit and submissions/commit across commit latency, max_in_flight and records-per-submission. Nothing writes parquet or touches object storage, so the numbers are not confounded by encode or upload cost — per-record CPU is measured separately by the bench package. A fixed injected delay looks like a reasonable stand-in for a real catalog here: measurement against a live engine-backed catalog found commit latency near-flat across a 667x range of batch sizes, so latency behaves as roughly constant with respect to batch size, and unlike a hosted service it can be swept across regimes rather than pinned to one. The sweep is flag-gated because it spends real wall time. TestCommitCoalescesConcurrentSubmissions pins the mechanism cheaply enough to run in CI: concurrent submissions arriving while a slow commit is in progress must merge into one subsequent commit rather than committing one at a time. Worth noting what this measures, because it bears on where to optimise next: the batcher already coalesces concurrent submissions maximally (eight submissions became one commit), and at max_in_flight=1 records-per-commit is pinned to a single submission by construction, since the only submitter is blocked inside the commit it is waiting on. So a time-based linger on the commit batcher looks like it would add nothing in the first case and could only add latency in the second. --- .../impl/iceberg/commit_regime_bench_test.go | 267 ++++++++++++++++++ 1 file changed, 267 insertions(+) create mode 100644 internal/impl/iceberg/commit_regime_bench_test.go diff --git a/internal/impl/iceberg/commit_regime_bench_test.go b/internal/impl/iceberg/commit_regime_bench_test.go new file mode 100644 index 0000000000..ac3047c552 --- /dev/null +++ b/internal/impl/iceberg/commit_regime_bench_test.go @@ -0,0 +1,267 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/redpanda/blob/master/licenses/rcl.md + +package iceberg + +import ( + "context" + "flag" + "fmt" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/redpanda-data/benthos/v4/public/service" +) + +// This harness isolates the COMMIT REGIME: how sink throughput responds to +// catalog commit latency, the number of concurrent in-flight submissions +// (max_in_flight), and the records carried per submission. It exists because +// the sink's headline throughput problem is latency-bound rather than +// CPU-bound, and the fix under evaluation (a commit linger) is a property of +// the commit batcher alone. +// +// It deliberately does NOT write parquet or touch object storage: files are +// synthesised metadata-only, so nothing here is confounded by encode or upload +// cost. Per-record CPU is measured separately by the bench/ package. +// +// Why a local latency injection is a faithful stand-in for a live catalog: +// measurement against a live Unity Catalog found commit latency near-flat +// across a 667x range of batch sizes (~5.2s at 300 records/commit rising only +// to ~9.7s at 200,000). Commit latency therefore behaves as a near-constant +// independent of batch size, which is exactly what a fixed injected delay +// models — with the advantage that the delay can be swept across regimes +// (a fast catalog at ~320ms, a slow engine-backed one at 5-10s) instead of +// being pinned to whatever one hosted service happens to do. + +var ( + regimeSweep = flag.Bool("iceberg.commit-regime", false, + "run the commit-regime sweep (takes minutes of wall time; prints a table)") + regimeRealistic = flag.Bool("iceberg.commit-regime-realistic", false, + "sweep at real engine-backed catalog latencies (5s/10s) instead of scaled-down ones") +) + +// latentCatalog wraps memCatalog with a fixed per-commit delay and counts +// commits, standing in for a catalog whose commit path costs real wall time +// (credential vending, metadata write, engine-side validation). +// +// The committer serialises all commits under commitMu, so CommitTable is never +// called concurrently; the counter is atomic only so readers can sample it +// while the sweep runs. +type latentCatalog struct { + *memCatalog + delay time.Duration + commits atomic.Int64 +} + +func (c *latentCatalog) CommitTable(ctx context.Context, ident table.Identifier, reqs []table.Requirement, updates []table.Update) (table.Metadata, string, error) { + if c.delay > 0 { + select { + case <-time.After(c.delay): + case <-ctx.Done(): + return nil, "", ctx.Err() + } + } + c.commits.Add(1) + return c.memCatalog.CommitTable(ctx, ident, reqs, updates) +} + +// regimeParams describes one point in the sweep. +type regimeParams struct { + commitLatency time.Duration // injected per-commit catalog cost + inFlight int // concurrent submitters, i.e. max_in_flight + recordsPerSubmit int // records carried by each submission + window time.Duration // measurement window +} + +// regimeResult is what one point measured. +type regimeResult struct { + params regimeParams + records int64 + submissions int64 + commits int64 + elapsed time.Duration + recordsPerSecond float64 + recordsPerCommit float64 + submitsPerCommit float64 +} + +func (r regimeResult) String() string { + return fmt.Sprintf("latency=%-6v in_flight=%-3d rec/submit=%-7d | rec/s=%-10.0f rec/commit=%-10.0f submits/commit=%-5.2f commits=%d", + r.params.commitLatency, r.params.inFlight, r.params.recordsPerSubmit, + r.recordsPerSecond, r.recordsPerCommit, r.submitsPerCommit, r.commits) +} + +// runRegime drives `inFlight` concurrent submitters against a committer whose +// catalog costs `commitLatency` per commit, for `window` of wall time, and +// reports what got through. Each submitter models one in-flight WriteBatch: +// build files, submit, block until the commit that carries them returns. +func runRegime(tb testing.TB, p regimeParams) regimeResult { + tb.Helper() + ctx := tb.Context() + + tbl, mem := newTestTable(tb) + cat := &latentCatalog{memCatalog: mem, delay: p.commitLatency} + + c, err := NewCommitter(tbl, cat, CommitConfig{ + ManifestMergeEnabled: false, + MaxRetries: 1, + }, func(context.Context) (*table.Table, error) { return cat.snapshot(), nil }, + service.MockResources().Logger()) + require.NoError(tb, err) + defer c.Close() + + var records, submissions atomic.Int64 + deadline := time.Now().Add(p.window) + schemaID := c.currentSchemaID() + + var wg sync.WaitGroup + start := time.Now() + for range p.inFlight { + wg.Go(func() { + for time.Now().Before(deadline) { + df := recordCountDataFile(tb, tbl.Spec(), + fmt.Sprintf("%s/data/%s.parquet", tbl.Location(), uuid.New()), + int64(p.recordsPerSubmit)) + if err := c.Commit(ctx, CommitInput{Files: []iceberg.DataFile{df}, SchemaID: schemaID}); err != nil { + return + } + submissions.Add(1) + records.Add(int64(p.recordsPerSubmit)) + } + }) + } + wg.Wait() + elapsed := time.Since(start) + + commits := cat.commits.Load() + res := regimeResult{ + params: p, + records: records.Load(), + submissions: submissions.Load(), + commits: commits, + elapsed: elapsed, + } + res.recordsPerSecond = float64(res.records) / elapsed.Seconds() + if commits > 0 { + res.recordsPerCommit = float64(res.records) / float64(commits) + res.submitsPerCommit = float64(res.submissions) / float64(commits) + } + return res +} + +// recordCountDataFile is synthDataFile with a caller-chosen record count, so a +// submission can represent a realistic batch rather than a single row. +func recordCountDataFile(tb testing.TB, spec iceberg.PartitionSpec, path string, records int64) iceberg.DataFile { + tb.Helper() + b, err := iceberg.NewDataFileBuilder( + spec, + iceberg.EntryContentData, + path, + iceberg.ParquetFile, + nil, nil, nil, + records, records*64, + ) + require.NoError(tb, err) + return b.Build() +} + +// TestCommitRegimeSweep characterises throughput across the commit regime. +// Flag-gated: it spends real wall time on purpose. +// +// The question it answers: does the existing batcher already coalesce +// concurrent submissions during a slow commit (in which case max_in_flight is +// the lever and a linger adds nothing), or does each commit carry only one +// submission (in which case a time-based linger is the fix)? +func TestCommitRegimeSweep(t *testing.T) { + if !*regimeSweep { + t.Skip("set -iceberg.commit-regime to run the commit-regime sweep") + } + + latencies := []time.Duration{50 * time.Millisecond, 200 * time.Millisecond, 500 * time.Millisecond} + window := 6 * time.Second + if *regimeRealistic { + latencies = []time.Duration{320 * time.Millisecond, 5 * time.Second, 10 * time.Second} + window = 60 * time.Second + } + + var results []regimeResult + for _, latency := range latencies { + for _, inFlight := range []int{1, 4, 16, 64} { + res := runRegime(t, regimeParams{ + commitLatency: latency, + inFlight: inFlight, + recordsPerSubmit: 300, // the observed "throughput trap" batch size + window: window, + }) + results = append(results, res) + t.Log(res.String()) + } + } + + t.Log("=== commit regime sweep (records/submit = 300) ===") + for _, r := range results { + t.Log(r.String()) + } +} + +// TestCommitCoalescesConcurrentSubmissions pins the mechanism the sweep +// explores, cheaply enough to run in CI: when several submissions are in +// flight while a slow commit is running, they must be merged into ONE +// subsequent commit rather than committed one at a time. +// +// This is the property any linger implementation must preserve — and the +// reason a linger cannot help at max_in_flight=1, where there is never a +// second submission to coalesce with. +func TestCommitCoalescesConcurrentSubmissions(t *testing.T) { + const ( + inFlight = 8 + latency = 300 * time.Millisecond + ) + ctx := t.Context() + + tbl, mem := newTestTable(t) + cat := &latentCatalog{memCatalog: mem, delay: latency} + + c, err := NewCommitter(tbl, cat, CommitConfig{ + ManifestMergeEnabled: false, + MaxRetries: 1, + }, func(context.Context) (*table.Table, error) { return cat.snapshot(), nil }, + service.MockResources().Logger()) + require.NoError(t, err) + defer c.Close() + + schemaID := c.currentSchemaID() + + // Occupy the committer so the rest of the submissions queue behind it. + var wg sync.WaitGroup + for i := range inFlight { + wg.Add(1) + go func(i int) { + defer wg.Done() + df := recordCountDataFile(t, tbl.Spec(), + fmt.Sprintf("%s/data/coalesce-%d-%s.parquet", tbl.Location(), i, uuid.New()), 300) + require.NoError(t, c.Commit(ctx, CommitInput{Files: []iceberg.DataFile{df}, SchemaID: schemaID})) + }(i) + } + wg.Wait() + + commits := cat.commits.Load() + require.Positive(t, commits, "expected at least one commit") + require.Less(t, commits, int64(inFlight), + "expected %d concurrent submissions to coalesce into fewer than %d commits, got %d", + inFlight, inFlight, commits) + t.Logf("%d concurrent submissions coalesced into %d commits (%.2f submissions/commit)", + inFlight, commits, float64(inFlight)/float64(commits)) +} From 1bf1770e91770f6e4aac6f45cc8dded239d6655e Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Thu, 20 Aug 2026 11:12:53 +0100 Subject: [PATCH 04/12] =?UTF-8?q?iceberg:=20address=20review=20=E2=80=94?= =?UTF-8?q?=20nested=20shredder=20coverage,=20goroutine=20assertions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from review feedback. The differential test between the two shredding paths was vacuous below the top level. It called shredStructExact and shredStructFolded directly on one case-sensitive shredder, but shredStruct dispatches on rs.caseSensitive on every recursion — so nested structs routed to shredStructExact for both runs and the test compared the fast path against itself. Nested divergence in unknown-field notification, required-field errors or path handling could not have been detected, which is precisely what the test exists to guard. Drive both runs through the public Shred entry point with two shredders instead, so one goes exact all the way down and the other folded all the way down. That comparison is only valid for a case-unambiguous corpus, so the doc-comment now states that every field name and key is lower-case and that case-differing input belongs in the case-sensitivity tests. Added three doubly-nested cases — an unknown key two levels down, and a required leaf missing and explicitly null two levels down — to reach the nested error and notification paths. Verified the test now has teeth by injecting a mutation that drops nested unknown-field notifications from the fast path only: both nested cases fail, where previously they could not have. Separately, the commit-regime harness asserted with require from its submitter goroutines. require calls FailNow, which is only valid on the goroutine running the test — it calls runtime.Goexit, so a commit failure unwound a submitter mid-loop and the test then evaluated its assertions against a half-finished run, reporting a confusing count mismatch instead of the actual error. Submitters now record their first error and the test goroutine asserts on errors.Join after waiting. The coalescing test builds its data files up front on the test goroutine so its submitters only commit. Both pass under -race. --- .../impl/iceberg/commit_regime_bench_test.go | 53 +++++++++++---- .../shredder/shredder_paths_agree_test.go | 67 ++++++++++++++++--- 2 files changed, 97 insertions(+), 23 deletions(-) diff --git a/internal/impl/iceberg/commit_regime_bench_test.go b/internal/impl/iceberg/commit_regime_bench_test.go index ac3047c552..d11e61650a 100644 --- a/internal/impl/iceberg/commit_regime_bench_test.go +++ b/internal/impl/iceberg/commit_regime_bench_test.go @@ -10,6 +10,7 @@ package iceberg import ( "context" + "errors" "flag" "fmt" "sync" @@ -126,15 +127,24 @@ func runRegime(tb testing.TB, p regimeParams) regimeResult { deadline := time.Now().Add(p.window) schemaID := c.currentSchemaID() + // Submitters cannot assert: require's FailNow is only valid on the test + // goroutine. Each records its first error for the test goroutine to check. + errs := make([]error, p.inFlight) + var wg sync.WaitGroup start := time.Now() - for range p.inFlight { + for i := range p.inFlight { wg.Go(func() { for time.Now().Before(deadline) { - df := recordCountDataFile(tb, tbl.Spec(), + df, err := recordCountDataFile(tbl.Spec(), fmt.Sprintf("%s/data/%s.parquet", tbl.Location(), uuid.New()), int64(p.recordsPerSubmit)) + if err != nil { + errs[i] = err + return + } if err := c.Commit(ctx, CommitInput{Files: []iceberg.DataFile{df}, SchemaID: schemaID}); err != nil { + errs[i] = err return } submissions.Add(1) @@ -145,6 +155,9 @@ func runRegime(tb testing.TB, p regimeParams) regimeResult { wg.Wait() elapsed := time.Since(start) + // Assert before deriving any rate from a possibly half-completed run. + require.NoError(tb, errors.Join(errs...), "submitter(s) failed") + commits := cat.commits.Load() res := regimeResult{ params: p, @@ -163,8 +176,12 @@ func runRegime(tb testing.TB, p regimeParams) regimeResult { // recordCountDataFile is synthDataFile with a caller-chosen record count, so a // submission can represent a realistic batch rather than a single row. -func recordCountDataFile(tb testing.TB, spec iceberg.PartitionSpec, path string, records int64) iceberg.DataFile { - tb.Helper() +// +// It returns an error rather than asserting, because the submitters that call it +// run on their own goroutines: require's FailNow is only valid on the goroutine +// running the test, so a failure here has to be carried back and asserted after +// the submitters have been waited on. +func recordCountDataFile(spec iceberg.PartitionSpec, path string, records int64) (iceberg.DataFile, error) { b, err := iceberg.NewDataFileBuilder( spec, iceberg.EntryContentData, @@ -173,8 +190,10 @@ func recordCountDataFile(tb testing.TB, spec iceberg.PartitionSpec, path string, nil, nil, nil, records, records*64, ) - require.NoError(tb, err) - return b.Build() + if err != nil { + return nil, err + } + return b.Build(), nil } // TestCommitRegimeSweep characterises throughput across the commit regime. @@ -244,18 +263,26 @@ func TestCommitCoalescesConcurrentSubmissions(t *testing.T) { schemaID := c.currentSchemaID() + // Build the data files up front, on the test goroutine, so the submitters + // below only have to commit — and so nothing in them needs to assert. + files := make([]iceberg.DataFile, inFlight) + for i := range files { + df, err := recordCountDataFile(tbl.Spec(), + fmt.Sprintf("%s/data/coalesce-%d-%s.parquet", tbl.Location(), i, uuid.New()), 300) + require.NoError(t, err) + files[i] = df + } + // Occupy the committer so the rest of the submissions queue behind it. + errs := make([]error, inFlight) var wg sync.WaitGroup for i := range inFlight { - wg.Add(1) - go func(i int) { - defer wg.Done() - df := recordCountDataFile(t, tbl.Spec(), - fmt.Sprintf("%s/data/coalesce-%d-%s.parquet", tbl.Location(), i, uuid.New()), 300) - require.NoError(t, c.Commit(ctx, CommitInput{Files: []iceberg.DataFile{df}, SchemaID: schemaID})) - }(i) + wg.Go(func() { + errs[i] = c.Commit(ctx, CommitInput{Files: []iceberg.DataFile{files[i]}, SchemaID: schemaID}) + }) } wg.Wait() + require.NoError(t, errors.Join(errs...), "submitter(s) failed") commits := cat.commits.Load() require.Positive(t, commits, "expected at least one commit") diff --git a/internal/impl/iceberg/shredder/shredder_paths_agree_test.go b/internal/impl/iceberg/shredder/shredder_paths_agree_test.go index 684818db3d..c0fb31cbae 100644 --- a/internal/impl/iceberg/shredder/shredder_paths_agree_test.go +++ b/internal/impl/iceberg/shredder/shredder_paths_agree_test.go @@ -24,8 +24,22 @@ import ( // key matching is exact — which is the default (case_sensitive_columns: true) // and therefore the path almost all traffic takes. It is an optimisation, so it // must not change observable behaviour by even one emitted value, new-field -// notification, or error. This test drives the same schema and record through -// both implementations and demands identical output. +// notification, or error. +// +// The comparison goes through the public Shred entry point with two shredders +// rather than calling the two helpers directly, because shredStruct dispatches +// on rs.caseSensitive on *every* recursion: a single case-sensitive shredder +// routes nested structs to shredStructExact no matter which helper was called +// at the top level, so calling the helpers directly would compare the exact +// path against itself below the root and silently pass on any nested +// divergence. Two shredders makes one run exact all the way down and the other +// folded all the way down. +// +// That comparison is only legitimate for a case-unambiguous corpus, so every +// schema field name and record key below is lower-case: folding then maps each +// key to itself and the two modes are *required* to agree. Inputs that differ +// in case are exactly where the modes are meant to diverge, and those belong in +// the case-sensitivity tests instead. // // It covers the cases the optimisation actually reasons about: every key // matching (the allocation-free steady state), extra unknown keys (the fallback @@ -43,11 +57,26 @@ func TestShredStructPathsAgree(t *testing.T) { Required: false, } + // deep carries a REQUIRED leaf two levels down, so nested required-field + // errors and nested unknown-field notifications are both reachable — the + // divergences the earlier version of this test could not have seen. + deep := iceberg.NestedField{ + ID: 20, + Name: "deep", + Type: &iceberg.StructType{FieldList: []iceberg.NestedField{ + {ID: 21, Name: "mid", Type: &iceberg.StructType{FieldList: []iceberg.NestedField{ + {ID: 22, Name: "leaf", Type: iceberg.PrimitiveTypes.String, Required: true}, + }}, Required: false}, + }}, + Required: false, + } + schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true}, iceberg.NestedField{ID: 2, Name: "name", Type: iceberg.PrimitiveTypes.String, Required: false}, iceberg.NestedField{ID: 3, Name: "flag", Type: iceberg.PrimitiveTypes.Bool, Required: false}, nested, + deep, ) emptySchema := iceberg.NewSchema(2) @@ -88,6 +117,30 @@ func TestShredStructPathsAgree(t *testing.T) { "inner": map[string]any{"a": int64(2), "nope": "surprise"}, }, }, + { + name: "unknown key in a doubly-nested struct", + schema: schema, + record: map[string]any{ + "id": int64(1), + "deep": map[string]any{"mid": map[string]any{"leaf": "ok", "extra": 1}}, + }, + }, + { + name: "required leaf missing two levels down (nested error path)", + schema: schema, + record: map[string]any{ + "id": int64(1), + "deep": map[string]any{"mid": map[string]any{}}, + }, + }, + { + name: "required leaf explicitly null two levels down", + schema: schema, + record: map[string]any{ + "id": int64(1), + "deep": map[string]any{"mid": map[string]any{"leaf": nil}}, + }, + }, { name: "missing optional fields", schema: schema, @@ -127,17 +180,11 @@ func TestShredStructPathsAgree(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - // Both shredders are case-sensitive: the folded path is exercised - // directly so the comparison isolates the implementations rather - // than the matching mode. - rs := NewRecordShredder(tc.schema, true) - fields := tc.schema.Fields() - exactSink := &testSink{} - exactErr := rs.shredStructExact(fields, tc.record, nil, 0, 0, 0, exactSink) + exactErr := NewRecordShredder(tc.schema, true).Shred(tc.record, exactSink) foldedSink := &testSink{} - foldedErr := rs.shredStructFolded(fields, tc.record, nil, 0, 0, 0, foldedSink) + foldedErr := NewRecordShredder(tc.schema, false).Shred(tc.record, foldedSink) if foldedErr != nil { require.EqualError(t, exactErr, foldedErr.Error(), From e9c8bd2c8edf351e78ecdc3988f44ae1725fd4c7 Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Thu, 20 Aug 2026 11:13:03 +0100 Subject: [PATCH 05/12] docs: record iceberg shredder and commit-regime benchmark results docs/benchmarking.md asks for a dated section in the results file whenever a connector's hot path changes, and for new bench harnesses to be documented and runnable from the directory's Taskfile. This PR did neither. Appends two sections to docs/benchmark-results/iceberg.md: the BenchmarkShredWide before/after for the shredder change (benchstat over n=8, with environment and PR link), and the commit-regime sweep. Both carry their reproduction command. The shredder section is explicit that the -66% is the micro-benchmark in isolation and that no sink-level throughput figure in the file has been re-measured, so the number is not mistaken for end-to-end. The commit-regime section carries a similar caveat: it writes no parquet and touches no object storage, so its rec/sec are ratios for comparing coalescing behaviour, not throughput comparable with the other sections. Also wires up the profiling configs, which were previously only usable by hand: bench:profile and bench:profile:schema run the two pipelines, and bench:shredder runs the micro-benchmark with no infrastructure. All three are documented in the bench README alongside the existing tasks. --- docs/benchmark-results/iceberg.md | 60 +++++++++++++++++++++++ internal/impl/iceberg/bench/README.md | 30 ++++++++++++ internal/impl/iceberg/bench/Taskfile.yaml | 46 +++++++++++++++++ 3 files changed, 136 insertions(+) diff --git a/docs/benchmark-results/iceberg.md b/docs/benchmark-results/iceberg.md index d0f4e23ad3..81315a4bfe 100644 --- a/docs/benchmark-results/iceberg.md +++ b/docs/benchmark-results/iceberg.md @@ -215,6 +215,66 @@ To reproduce: the localhost benchmark configs live under [`internal/impl/iceberg --- +## Shredder Allocations — 2026-08-20 + +Record shredding (JSON `map[string]any` → columnar parquet values) built two maps per struct per record to support case-insensitive key matching. Case-sensitive matching is the default (`case_sensitive_columns: true`) and makes those maps redundant, so it now has a dedicated path that looks fields up directly and skips unknown-field scanning when every input key is accounted for. + +Driven by `BenchmarkShredWide` in [`internal/impl/iceberg/bench/`](../../internal/impl/iceberg/bench/) — a wide-schema shredder micro-benchmark that mirrors the profiling pipeline's record shape without standing up infrastructure. + +**Environment:** darwin/arm64, Apple M3 Pro, `GOMAXPROCS=1`, Go benchmark, `benchstat` over n=8 + +**Changed since last run:** the case-sensitive shredding path ([#4712](https://github.com/redpanda-data/connect/pull/4712)). No configuration or behaviour change. + +| metric | before | after | delta | +|-----------|---------|---------|-------------------| +| sec/op | 4.369µs | 1.472µs | **-66.3%** (p=0.000) | +| B/op | 4.312 KiB | 1.609 KiB | **-62.7%** (p=0.000) | +| allocs/op | 71 | 41 | **-42.3%** (p=0.000) | + +Per sub-benchmark, sec/op: `declared_schema=false` 4.304µs → 1.394µs (-67.6%); `declared_schema=true` 4.435µs → 1.555µs (-64.9%). + +**Observations:** + +- **This is the shredder in isolation, not a sink-level number.** Earlier 1-vCPU profiling attributed ~27% of the sink's CPU to shredding, so the end-to-end effect should be appreciable but much smaller than 66%. **It has not been measured end to end** — no throughput figure above or elsewhere in this file has been re-run for this change. +- The two `declared_schema` variants are within noise of each other both before and after, consistent with the earlier finding that the `schema_metadata` knob does not bypass decode, shredding or encode. + +To reproduce: `GOMAXPROCS=1 go test -bench BenchmarkShredWide -benchmem -run '^$' -count=8 ./internal/impl/iceberg/bench/` + +--- + +## Commit Regime — Commit Latency vs `max_in_flight` (synthetic) + +How commit coalescing responds to catalog commit latency and the number of concurrent in-flight submissions, measured by the flag-gated `TestCommitRegimeSweep` in [`internal/impl/iceberg/commit_regime_bench_test.go`](../../internal/impl/iceberg/commit_regime_bench_test.go). + +**Environment:** darwin/arm64, Apple M3 Pro; in-memory catalog with a fixed injected per-commit delay; 6s window per point; 300 records per submission + +**Caveat — read the numbers as ratios, not throughput.** Nothing here writes parquet or touches object storage, and the injected delay is not a real catalog, so the absolute rec/sec are not sink throughput figures and are not comparable with the localhost or live-catalog sections above. What the harness measures is how many submissions a commit carries, and at what latency. + +| commit latency | `max_in_flight` | rec/sec | records/commit | submissions/commit | +|---------------:|----------------:|--------:|---------------:|-------------------:| +| 50ms | 1 | 5,238 | 300 | 1.00 | +| 50ms | 4 | 10,437 | 600 | 2.00 | +| 50ms | 16 | 41,790 | 2,400 | 8.00 | +| 50ms | 64 | 166,306 | 9,600 | 32.00 | +| 200ms | 1 | 1,449 | 300 | 1.00 | +| 200ms | 4 | 2,896 | 600 | 2.00 | +| 200ms | 16 | 11,563 | 2,400 | 8.00 | +| 200ms | 64 | 46,230 | 9,600 | 32.00 | +| 500ms | 1 | 591 | 300 | 1.00 | +| 500ms | 4 | 1,187 | 600 | 2.00 | +| 500ms | 16 | 4,416 | 2,238 | 7.46 | +| 500ms | 64 | 20,354 | 10,338 | 34.46 | + +**Observations:** + +- **The commit batcher already coalesces concurrent submissions.** Submissions that arrive while a commit is in flight are merged into the next one, so records per commit scales with `max_in_flight` without any time-based batching involved. +- **At `max_in_flight: 1` records per commit is pinned to a single submission**, giving `records-per-submission / commit-latency` — 591 rec/sec at 500ms, matching the "throughput trap" regime described under Tuning Recipes. This is structural: the sole submitter is blocked inside the commit it is waiting on, so no second submission can exist to batch with. A commit-side linger cannot improve this case, and would add latency to it. +- Submissions per commit settles near `max_in_flight / 2` rather than `max_in_flight`, which suggests the batcher samples its queue before the just-released submitters have all re-queued. Whether closing that gap is worth anything is untested. + +To reproduce: `go test -run TestCommitRegimeSweep -iceberg.commit-regime -timeout 20m ./internal/impl/iceberg/` (add `-iceberg.commit-regime-realistic` for 320ms/5s/10s latencies). + +--- + ## Tuning Recipes The single most important factor for `iceberg` throughput is **records per commit**. Each catalog diff --git a/internal/impl/iceberg/bench/README.md b/internal/impl/iceberg/bench/README.md index 69c24db2c7..3f75c6badc 100644 --- a/internal/impl/iceberg/bench/README.md +++ b/internal/impl/iceberg/bench/README.md @@ -47,6 +47,36 @@ task bench:mif CORES=4 BATCH=10000 MIF=32 COUNT=1000000 |-----------|---------|-------------| | `MIF` | 4 | `max_in_flight` | +### Profiling (per-record CPU) + +`profile_config.yaml` is a profiling variant of `benchmark_config.yaml`: a ~1.2 kB +high-entropy JSON payload serialised to raw bytes in the pipeline, so the Iceberg +output performs a real JSON parse per record and the profile attributes decode, +shredding and encode separately. `profile_config_schema.yaml` is the same +pipeline with a declared schema, for measuring what `schema_metadata` buys. + +```bash +task bench:profile CORES=1 COUNT=500000 # schemaless +task bench:profile:schema CORES=1 COUNT=500000 # declared schema +``` + +| Parameter | Default | Description | +|-----------|---------|-------------| +| `CORES` | 1 | `GOMAXPROCS` — 1 isolates per-record CPU cost | +| `BATCH` | 5000 | `batching.count` | +| `COUNT` | 500000 | number of messages | + +### Shredder micro-benchmark + +Needs no infrastructure — it exercises the shredder directly: + +```bash +task bench:shredder # GOMAXPROCS=1, -count=8 +``` + +Results are recorded in +[`docs/benchmark-results/iceberg.md`](../../../../docs/benchmark-results/iceberg.md). + ### Clean run ```bash diff --git a/internal/impl/iceberg/bench/Taskfile.yaml b/internal/impl/iceberg/bench/Taskfile.yaml index 4035bdd07c..5bcc729a9c 100644 --- a/internal/impl/iceberg/bench/Taskfile.yaml +++ b/internal/impl/iceberg/bench/Taskfile.yaml @@ -85,6 +85,52 @@ tasks: --set output.iceberg.storage.aws_s3.credentials.secret={{.MINIO_PASSWORD}} \ ./benchmark_config.yaml + # Profiling run — same pipeline as `bench` but with the ~1.2kB high-entropy + # payload from profile_config.yaml, parsed per record so the sink does real + # JSON decode + shredding + encode work. Writes pprof profiles for attributing + # per-record CPU. + # Usage: task bench:profile CORES=1 BATCH=5000 COUNT=500000 + bench:profile: + desc: "Run the profiling pipeline and write CPU/heap profiles (e.g. task bench:profile CORES=1 COUNT=500000)" + vars: + CORES: '{{.CORES | default "1"}}' + BATCH: '{{.BATCH | default "5000"}}' + COUNT: '{{.COUNT | default "500000"}}' + CONFIG: '{{.CONFIG | default "./profile_config.yaml"}}' + cmds: + - | + AWS_EC2_METADATA_DISABLED=true \ + AWS_ACCESS_KEY_ID={{.MINIO_USER}} \ + AWS_SECRET_ACCESS_KEY={{.MINIO_PASSWORD}} \ + AWS_REGION={{.MINIO_REGION}} \ + GOMAXPROCS={{.CORES}} go run ../../../../cmd/redpanda-connect/main.go run \ + --set input.generate.count={{.COUNT}} \ + --set output.iceberg.batching.count={{.BATCH}} \ + --set output.iceberg.catalog.url={{.CATALOG_URL}} \ + --set output.iceberg.storage.aws_s3.endpoint={{.MINIO_ENDPOINT}} \ + --set output.iceberg.storage.aws_s3.bucket={{.MINIO_BUCKET}} \ + --set output.iceberg.storage.aws_s3.credentials.id={{.MINIO_USER}} \ + --set output.iceberg.storage.aws_s3.credentials.secret={{.MINIO_PASSWORD}} \ + {{.CONFIG}} + + # Same pipeline with a declared schema, to measure what schema_metadata buys. + # Usage: task bench:profile:schema CORES=1 COUNT=500000 + bench:profile:schema: + desc: "Run the declared-schema profiling pipeline (e.g. task bench:profile:schema CORES=1 COUNT=500000)" + cmds: + - task: bench:profile + vars: + CONFIG: ./profile_config_schema.yaml + CORES: '{{.CORES | default "1"}}' + BATCH: '{{.BATCH | default "5000"}}' + COUNT: '{{.COUNT | default "500000"}}' + + # Shredder micro-benchmark — no infrastructure required. + bench:shredder: + desc: Run the wide-schema shredder micro-benchmark (no infra needed) + cmds: + - GOMAXPROCS={{.CORES | default "1"}} go test -bench BenchmarkShredWide -benchmem -run '^$' -count={{.COUNT | default "8"}} . + bench:lag: desc: Show current consumer lag for the Redpanda Connect Iceberg sink group cmds: From 36489615ac61824522832479fbf6611c2c4575e1 Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Tue, 25 Aug 2026 17:09:35 +0100 Subject: [PATCH 06/12] iceberg: add an optional parquet compression codec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Data files were always written uncompressed: the only writer option this output ever passed was a string encoding, so parquet-go fell back to its uncompressed default. Nothing consulted the table's own write.parquet.compression-codec property either. Add a `parquet.compression` field accepting uncompressed, snappy, gzip and zstd. It is deliberately optional rather than defaulted, so that "unset" is distinguishable from an explicit "uncompressed" and can mean "defer to the table". Resolution per table, once, when its writer is built: 1. parquet.compression, when set 2. otherwise the table's write.parquet.compression-codec property 3. otherwise uncompressed, preserving existing behaviour Only codecs every targeted engine reads are offered. The omissions are interoperability hazards rather than gaps: the original LZ4 codec was ambiguously specified (Hadoop framing vs raw blocks) and readers disagree on which one it means, while brotli and lzo have patchy engine support. A table property naming one of those is reported and treated as unset rather than failing the write — the property is not this output's configuration to validate, and uncompressed is readable everywhere. Documented as its own section because two things surprise: the precedence above, and that the property is the better lever. Copy-on-write rewrites whole data files from inside the Iceberg library, out of reach of a writer option we pass, but that code reads the same property — and defaults it to zstd. So an otherwise unconfigured table already holds a mixture today, uncompressed appends alongside zstd copy-on-write rewrites. That is legal and transparent to readers, since parquet records its codec per column chunk, but it means setting the property makes every file agree whereas setting the field governs appends and merge-on-read only. The field remains the escape hatch for catalogs that reject client-set table properties. Tests cover the resolution order, both spellings of no compression ("none" and "uncompressed"), the declined codecs, and unknown and empty property values. A second test asserts the resolved codec reaches the written bytes, by reading the codec back out of each column chunk in the footer rather than trusting the resolver's return value. --- .../components/pages/outputs/iceberg.adoc | 40 +++++ internal/impl/iceberg/config.go | 32 +++- internal/impl/iceberg/output_iceberg.go | 11 ++ internal/impl/iceberg/parquet_compression.go | 99 +++++++++++ .../impl/iceberg/parquet_compression_test.go | 157 ++++++++++++++++++ internal/impl/iceberg/router.go | 17 +- 6 files changed, 354 insertions(+), 2 deletions(-) create mode 100644 internal/impl/iceberg/parquet_compression.go create mode 100644 internal/impl/iceberg/parquet_compression_test.go diff --git a/docs/modules/components/pages/outputs/iceberg.adoc b/docs/modules/components/pages/outputs/iceberg.adoc index d355237fca..6597c17ecd 100644 --- a/docs/modules/components/pages/outputs/iceberg.adoc +++ b/docs/modules/components/pages/outputs/iceberg.adoc @@ -160,6 +160,7 @@ output: cleanup_on_failure: true parquet: string_encoding: delta_length_byte_array + compression: "" # No default (optional) batching: count: 0 byte_size: 0 @@ -299,6 +300,24 @@ To guarantee an existing table never ends up with a mix of the two annotations, A table pinned `legacy` keeps receiving the legacy annotation on every new file — byte-identical to what previous releases wrote — so appends and `merge-on-read` continue working unchanged forever. The one restriction is mutating `copy-on-write` (`upsert`/`delete`): it must rewrite existing files, which the legacy annotation prevents, so such writes fail upfront with an actionable error (pure `insert` batches still work). To migrate a legacy table to the spec encoding: rewrite/compact the table's data files with an engine that writes the spec annotation (e.g. Spark's `rewrite_data_files`), then set the table property `redpanda-connect.timestamp-encoding` to `spec`, keeping any running instances of this output that write to the table stopped (or restarting them) around the migration — a live writer only re-reads the property when its writer is recreated. A table whose existing files already mix both annotations (for example one written to by several engines or connector versions over time) can be pinned either way by the probe, depending on which file it happens to read first, and a `copy-on-write` mutation on such a table may then fail mid-rewrite with the underlying library's type-promotion error rather than the upfront migration message — compact or rewrite such a table to a single encoding before mutating it. Alternatively, keep the table on `merge-on-read`. +== Data file compression + +Compression of the parquet data files this output writes is resolved per table, in this order: + +1. `parquet.compression`, if you set it. +2. otherwise the table's own `write.parquet.compression-codec` property, if the table has one. +3. otherwise uncompressed. + +`parquet.compression` is optional rather than defaulted precisely so that step 2 is reachable: an unset field means "whatever the table says", which is not the same as explicitly choosing `uncompressed`. + +*Prefer the table property where you can.* Mutations written with `merge_strategy: copy-on-write` rewrite whole data files, and that rewrite happens inside the Iceberg library rather than in this output's own writer — so `parquet.compression` does not reach it, but the table property does (the library reads the same property, and defaults it to `zstd` when absent). Setting the property therefore makes every file this output writes agree with itself; setting the field governs appends and merge-on-read writes only. Reach for the field when the property is not available to you — notably on catalogs that reject client-set table properties, such as the Databricks Unity Catalog. + +NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: appended files are uncompressed, while any copy-on-write rewrites are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data. + +*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, while `brotli` and `lzo` have patchy engine support. If the table property names one of those, it is reported at startup and files are written uncompressed rather than risking data an engine refuses to read. + +*Cost.* Compression trades CPU for bytes, and this output is per-record CPU bound at low core counts, so enabling it on a small deployment can cost throughput. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most. + == Performance @@ -1209,6 +1228,27 @@ Options: , `delta_length_byte_array` . +=== `parquet.compression` + +The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed. + +Setting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property makes every file this output writes agree, whereas this field only governs appends and merge-on-read writes. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties. + +Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `brotli`, `lzo`), it is reported at startup and data files are written uncompressed rather than risking files a reader cannot open. + +Compression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat. + + +*Type*: `string` + + +Options: +`uncompressed` +, `snappy` +, `gzip` +, `zstd` +. + === `batching` Allows you to configure a xref:configuration:batching.adoc[batching policy]. diff --git a/internal/impl/iceberg/config.go b/internal/impl/iceberg/config.go index 794ba34c21..98982ee99d 100644 --- a/internal/impl/iceberg/config.go +++ b/internal/impl/iceberg/config.go @@ -99,8 +99,33 @@ const ( // Parquet writer fields ioFieldParquet = "parquet" ioFieldParquetStringEncoding = "string_encoding" + ioFieldParquetCompression = "compression" ) +// compressionDocs is the long-form documentation for data-file compression. It +// lives in the component description rather than only in the `compression` +// field because the interesting part is a three-way precedence rule plus an +// asymmetry between this output's two write paths, which reads poorly in a +// field table cell. +const compressionDocs = "\n" + + "== Data file compression\n" + + "\n" + + "Compression of the parquet data files this output writes is resolved per table, in this order:\n" + + "\n" + + "1. `parquet.compression`, if you set it.\n" + + "2. otherwise the table's own `write.parquet.compression-codec` property, if the table has one.\n" + + "3. otherwise uncompressed.\n" + + "\n" + + "`parquet.compression` is optional rather than defaulted precisely so that step 2 is reachable: an unset field means \"whatever the table says\", which is not the same as explicitly choosing `uncompressed`.\n" + + "\n" + + "*Prefer the table property where you can.* Mutations written with `merge_strategy: copy-on-write` rewrite whole data files, and that rewrite happens inside the Iceberg library rather than in this output's own writer — so `parquet.compression` does not reach it, but the table property does (the library reads the same property, and defaults it to `zstd` when absent). Setting the property therefore makes every file this output writes agree with itself; setting the field governs appends and merge-on-read writes only. Reach for the field when the property is not available to you — notably on catalogs that reject client-set table properties, such as the Databricks Unity Catalog.\n" + + "\n" + + "NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: appended files are uncompressed, while any copy-on-write rewrites are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data.\n" + + "\n" + + "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, while `brotli` and `lzo` have patchy engine support. If the table property names one of those, it is reported at startup and files are written uncompressed rather than risking data an engine refuses to read.\n" + + "\n" + + "*Cost.* Compression trades CPU for bytes, and this output is per-record CPU bound at low core counts, so enabling it on a small deployment can cost throughput. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most.\n" + // rowOperationDocs is the long-form documentation for the row-level operation // feature. It lives in the component description rather than inline in the // `row_operation` / `identifier_fields` field descriptions because the field @@ -243,7 +268,7 @@ object:struct array:list |=== -`+rowOperationDocs+service.OutputPerformanceDocs(true, true)). +`+rowOperationDocs+compressionDocs+service.OutputPerformanceDocs(true, true)). Fields( // Catalog configuration service.NewObjectField(ioFieldCatalog, @@ -497,6 +522,11 @@ array:list Description("The encoding to use for string and binary columns. Use `plain` for compatibility with readers that do not support `DELTA_LENGTH_BYTE_ARRAY` encoding, such as AWS Redshift Spectrum."). ShortDescription("Encoding for string and binary columns. Use plain for readers lacking DELTA_LENGTH_BYTE_ARRAY."). Default("delta_length_byte_array"), + service.NewStringEnumField(ioFieldParquetCompression, + "uncompressed", "snappy", "gzip", "zstd"). + Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property makes every file this output writes agree, whereas this field only governs appends and merge-on-read writes. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `brotli`, `lzo`), it is reported at startup and data files are written uncompressed rather than risking files a reader cannot open.\n\nCompression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat."). + ShortDescription("Compression codec for written data files. Defaults to the table's write.parquet.compression-codec property, else uncompressed."). + Optional(), ).Description("Parquet writer configuration."). Advanced(). Optional(), diff --git a/internal/impl/iceberg/output_iceberg.go b/internal/impl/iceberg/output_iceberg.go index 08dd3d42e7..aab2a8a5e8 100644 --- a/internal/impl/iceberg/output_iceberg.go +++ b/internal/impl/iceberg/output_iceberg.go @@ -210,8 +210,19 @@ func newIcebergOutputFromConfig(conf *service.ParsedConfig, mgr *service.Resourc } } + // Compression is deliberately optional: unset means "defer to the table's + // write.parquet.compression-codec property, else uncompressed", which is + // resolved per table when its writer is built (resolveParquetCompression). + var parquetCompression string + if conf.Contains(ioFieldParquet, ioFieldParquetCompression) { + if parquetCompression, err = conf.FieldString(ioFieldParquet, ioFieldParquetCompression); err != nil { + return nil, fmt.Errorf("parsing %s: %w", ioFieldParquetCompression, err) + } + } + rtr := NewRouter(catalogCfg, namespaceStr, tableStr, caseSensitive, schemaEvoCfg, commitCfg, rowOpCfg, writerOpts, mgr.Logger()) rtr.metrics = newOpMetrics(mgr.Metrics()) + rtr.parquetCompression = parquetCompression return &icebergOutput{ router: rtr, logger: mgr.Logger(), diff --git a/internal/impl/iceberg/parquet_compression.go b/internal/impl/iceberg/parquet_compression.go new file mode 100644 index 0000000000..1e3716b9ff --- /dev/null +++ b/internal/impl/iceberg/parquet_compression.go @@ -0,0 +1,99 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/redpanda/blob/master/licenses/rcl.md + +package iceberg + +import ( + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" + "github.com/parquet-go/parquet-go" + "github.com/parquet-go/parquet-go/compress" + + "github.com/redpanda-data/benthos/v4/public/service" +) + +// Compression codec names accepted by the `parquet.compression` field. These +// are deliberately a subset of the codecs parquet permits: every one of them is +// read by all the query engines this output targets. The codecs left out are +// interoperability hazards rather than technical gaps — +// +// - lz4: the original LZ4 codec was ambiguously specified (Hadoop framing vs. +// raw blocks) and readers disagree on which one `LZ4` means, which is why +// LZ4_RAW was later added to the format. Writing either risks files a given +// engine refuses. +// - brotli, lzo: read support across engines is patchy. +// +// A table property may still name one of those (see resolveParquetCompression), +// in which case this output declines to write it rather than produce files some +// reader cannot open. +const ( + compressionUncompressed = "uncompressed" + compressionSnappy = "snappy" + compressionGzip = "gzip" + compressionZstd = "zstd" +) + +// parquetCompressionCodecs maps an accepted codec name to its parquet-go codec. +// "none" is included because that is the spelling Iceberg's own table property +// uses for no compression, and the property is a valid source of this value. +var parquetCompressionCodecs = map[string]compress.Codec{ + compressionUncompressed: &parquet.Uncompressed, + "none": &parquet.Uncompressed, + compressionSnappy: &parquet.Snappy, + compressionGzip: &parquet.Gzip, + compressionZstd: &parquet.Zstd, +} + +// resolveParquetCompression decides which compression codec this output writes +// data files with for one table, resolving in a fixed order: +// +// 1. `parquet.compression`, when set — an explicit operator instruction wins. +// 2. the table's own `write.parquet.compression-codec` property, so a table +// configured by its owner (or another writer) is honoured without needing +// connector configuration. Also the only way to make the copy-on-write +// rewrite path agree, since that path is inside iceberg-go and reads this +// property itself. +// 3. uncompressed, preserving this output's historical behaviour when neither +// is specified. +// +// A property naming a codec this output declines to write (see the constants +// above) is reported and treated as unset rather than failing the write: the +// property is not this output's configuration to validate, and uncompressed is +// readable everywhere, so refusing to start would be a worse outcome than +// writing data every engine can read. An invalid *configured* value cannot +// reach here — the config field is an enum, validated at startup. +// +// Called once per table when its writer is built, not per batch. +func resolveParquetCompression(configured string, props iceberg.Properties, logger *service.Logger) compress.Codec { + if configured != "" { + if codec, ok := parquetCompressionCodecs[configured]; ok { + return codec + } + // Unreachable via config validation; be explicit rather than silently + // writing something the operator did not ask for. + if logger != nil { + logger.Warnf("Unsupported %s.%s value %q; writing uncompressed data files.", ioFieldParquet, ioFieldParquetCompression, configured) + } + return &parquet.Uncompressed + } + + fromTable, ok := props[table.ParquetCompressionKey] + if !ok || fromTable == "" { + return &parquet.Uncompressed + } + + if codec, ok := parquetCompressionCodecs[fromTable]; ok { + return codec + } + + if logger != nil { + logger.Warnf("Table property %s is %q, which this output does not write (readers disagree on it or engine support is patchy); writing uncompressed data files instead. Set %s.%s to choose a supported codec explicitly.", + table.ParquetCompressionKey, fromTable, ioFieldParquet, ioFieldParquetCompression) + } + return &parquet.Uncompressed +} diff --git a/internal/impl/iceberg/parquet_compression_test.go b/internal/impl/iceberg/parquet_compression_test.go new file mode 100644 index 0000000000..34b0d1806c --- /dev/null +++ b/internal/impl/iceberg/parquet_compression_test.go @@ -0,0 +1,157 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/redpanda/blob/master/licenses/rcl.md + +package iceberg + +import ( + "testing" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" + "github.com/parquet-go/parquet-go" + "github.com/parquet-go/parquet-go/format" + "github.com/stretchr/testify/require" + + "github.com/redpanda-data/benthos/v4/public/service" + + "github.com/redpanda-data/connect/v4/internal/impl/iceberg/icebergx" + "github.com/redpanda-data/connect/v4/internal/impl/iceberg/shredder" +) + +// TestResolveParquetCompression pins the resolution order: configured value, +// then the table's own property, then uncompressed. +func TestResolveParquetCompression(t *testing.T) { + logger := service.MockResources().Logger() + + tests := []struct { + name string + configured string + props iceberg.Properties + want format.CompressionCodec + }{ + { + name: "nothing set writes uncompressed", + want: format.Uncompressed, + }, + { + name: "configured value is used", + configured: compressionZstd, + want: format.Zstd, + }, + { + name: "table property is used when unset", + props: iceberg.Properties{table.ParquetCompressionKey: "snappy"}, + want: format.Snappy, + }, + { + name: "configured value beats the table property", + configured: compressionSnappy, + props: iceberg.Properties{table.ParquetCompressionKey: "zstd"}, + want: format.Snappy, + }, + { + // "none" is Iceberg's spelling for no compression; it must not be + // mistaken for an unrecognised value. + name: "table property none is recognised", + props: iceberg.Properties{table.ParquetCompressionKey: "none"}, + want: format.Uncompressed, + }, + { + name: "table property uncompressed is recognised", + props: iceberg.Properties{table.ParquetCompressionKey: "uncompressed"}, + want: format.Uncompressed, + }, + { + name: "gzip from the table property", + props: iceberg.Properties{table.ParquetCompressionKey: "gzip"}, + want: format.Gzip, + }, + { + // The codecs deliberately not offered: honouring them would risk + // files some reader refuses, so they degrade to uncompressed rather + // than failing the write. + name: "lz4 from the table property degrades to uncompressed", + props: iceberg.Properties{table.ParquetCompressionKey: "lz4"}, + want: format.Uncompressed, + }, + { + name: "brotli from the table property degrades to uncompressed", + props: iceberg.Properties{table.ParquetCompressionKey: "brotli"}, + want: format.Uncompressed, + }, + { + name: "unknown table property value degrades to uncompressed", + props: iceberg.Properties{table.ParquetCompressionKey: "nonsense"}, + want: format.Uncompressed, + }, + { + name: "empty table property value is treated as unset", + props: iceberg.Properties{table.ParquetCompressionKey: ""}, + want: format.Uncompressed, + }, + { + name: "an unrelated property does not interfere", + props: iceberg.Properties{"write.metadata.compression-codec": "gzip"}, + want: format.Uncompressed, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := resolveParquetCompression(tc.configured, tc.props, logger) + require.Equal(t, tc.want, got.CompressionCodec()) + }) + } +} + +// TestParquetCompressionReachesWrittenFile checks the resolved codec actually +// governs the bytes on disk, not merely what the resolver returns: the option +// has to survive being handed to the sink and applied per column chunk. +func TestParquetCompressionReachesWrittenFile(t *testing.T) { + sc := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64}, + iceberg.NestedField{ID: 2, Name: "s", Type: iceberg.PrimitiveTypes.String}, + ) + + for _, tc := range []struct { + name string + configured string + props iceberg.Properties + want format.CompressionCodec + }{ + {name: "default is uncompressed", want: format.Uncompressed}, + {name: "configured zstd", configured: compressionZstd, want: format.Zstd}, + {name: "property snappy", props: iceberg.Properties{table.ParquetCompressionKey: "snappy"}, want: format.Snappy}, + } { + t.Run(tc.name, func(t *testing.T) { + pqSchema, fieldToCol, err := icebergx.BuildParquetSchema(sc, icebergx.TimestampEncoding(0)) + require.NoError(t, err) + + codec := resolveParquetCompression(tc.configured, tc.props, service.MockResources().Logger()) + sink := newParquetSink(pqSchema, fieldToCol, true, parquet.Compression(codec)) + + // Enough rows of repetitive data that a codec has something to bite + // on, driven the way the writer drives it: shred a row, then flush. + for i := range 500 { + require.NoError(t, sink.EmitValue(shredder.ShreddedValue{FieldID: 1, Value: parquet.ValueOf(int64(i))})) + require.NoError(t, sink.EmitValue(shredder.ShreddedValue{FieldID: 2, Value: parquet.ValueOf("a highly compressible repeated string value")})) + require.NoError(t, sink.flush()) + } + res, err := sink.Close() + require.NoError(t, err) + + require.NotEmpty(t, res.footer.RowGroups, "expected at least one row group") + for _, rg := range res.footer.RowGroups { + for _, col := range rg.Columns { + require.Equal(t, tc.want, col.MetaData.Codec, + "column %v written with the wrong codec", col.MetaData.PathInSchema) + } + } + }) + } +} diff --git a/internal/impl/iceberg/router.go b/internal/impl/iceberg/router.go index 5e1d42ddbe..5e62981b64 100644 --- a/internal/impl/iceberg/router.go +++ b/internal/impl/iceberg/router.go @@ -147,6 +147,13 @@ type Router struct { entries sync.Map // tableKey -> *tableEntry + // parquetCompression is the configured `parquet.compression` value, or "" + // when unset. Set after construction by the output, like metrics below, so + // NewRouter's signature stays put. Empty means each table falls back to its + // own write.parquet.compression-codec property — see + // resolveParquetCompression. + parquetCompression string + // metrics is optional (nil in some tests); set after construction by the // output. Writers and committers inherit it. metrics *opMetrics @@ -912,7 +919,15 @@ func (r *Router) createWriter(ctx context.Context, key tableKey, entry *tableEnt } } - w := NewWriter(writerTbl, comm, r.caseSensitive, r.writerOpts, r.resolver, r.schemaEvoCfg.RequireSchemaMetadata, r.rowOpCfg, entry.tsEncoding, r.logger) + // Resolve compression per table, since the fallback reads that table's own + // property. slices.Concat rather than append: appending to r.writerOpts + // would share its backing array between tables, so two tables resolving to + // different codecs could overwrite each other's option. + writerOpts := slices.Concat(r.writerOpts, []parquet.WriterOption{ + parquet.Compression(resolveParquetCompression(r.parquetCompression, writerTbl.Properties(), r.logger)), + }) + + w := NewWriter(writerTbl, comm, r.caseSensitive, writerOpts, r.resolver, r.schemaEvoCfg.RequireSchemaMetadata, r.rowOpCfg, entry.tsEncoding, r.logger) w.metrics = r.metrics r.logger.Debugf("Created writer for table %s.%s", key.namespace, key.table) From ad6056f461a2a73d9544e487e5b4090871f866ad Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Wed, 26 Aug 2026 09:52:15 +0100 Subject: [PATCH 07/12] =?UTF-8?q?iceberg:=20address=20review=20=E2=80=94?= =?UTF-8?q?=20codec=20casing,=20duplicate=20field=20names,=20doc=20reach?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback plus two bugs found by an independent pass over the same code. Compression codec names are now folded before lookup, on BOTH sides. The table property obviously needs it — it belongs to whoever owns the table, so its casing is not ours to dictate. The configured value turned out to need it too, and less obviously: the config framework's enum linter lower-cases before comparing against the option set, so `compression: ZSTD` passes validation and arrives verbatim. It previously missed the map and silently wrote uncompressed while logging a warning that read as an internal-invariant violation. The comment claiming an invalid configured value could not reach the resolver was therefore wrong, and is gone. The shredder's fast path had a real divergence from the general path. Its unknown-field shortcut compares a count of matched fields against the number of input keys, which assumes field names are unique within a struct — an assumption the old comment asserted and nothing enforces. Iceberg rejects duplicate field IDs but not duplicate names, and in case-sensitive mode nothing upstream checks them either. Fields [a, a] against {"a":…, "b":…} reach matchedKeys == len(value) while "b" is genuinely unknown, so the fast path skipped the scan and never reported it — losing the schema evolution of that column, silently. Duplicate names are now detected once per shredder (walking through list elements and map values too) and disable the shortcut for that schema, leaving the optimisation intact for every well-formed one. Compression warnings are returned by the resolver rather than logged inside it, so the router can suppress repeats. Writers are rebuilt on every write failure, so a retrying pipeline against a table whose property names an unwritable codec would otherwise have emitted the same warning without bound. The docs said "at startup", which was never accurate. Docs corrected where they overstated the field's reach: equality-delete files are written by the Iceberg library, like copy-on-write rewrites, so the field governs appends and merge-on-read *data* files only. Added that the library's own codec lookup is lower-case only, so a property of `ZSTD` yields compressed appends and uncompressed rewrites — use lower case. lz4raw is now listed among the declined codecs, with the accurate reason: it is the unambiguous replacement for lz4, so the ambiguity argument does not apply to it, but its reader support is younger and less universal. Tests: casing variants on both the configured value and the property, whitespace-only property values, warning text, the duplicate-name divergence (verified to fail without the guard), the detection walk through lists and maps, struct-in-list and struct-in-map agreement cases, and a comparison of values emitted before an error rather than only the error itself. Added a case-insensitive shredder benchmark, which caught a ~5% regression from briefly extracting that path into its own function; it is inlined again and the regression is gone (p=0.604 vs the pre-change baseline). Harness: commits are counted after completion rather than on attempt, the coalescing assertion is tightened from "fewer than 8" to "at most 2" so a regression to near-no-coalescing actually fails, and the window's overshoot is documented. Benchmark results sections carry the date and PR link the benchmarking docs require. --- docs/benchmark-results/iceberg.md | 4 +- .../components/pages/outputs/iceberg.adoc | 12 +- .../impl/iceberg/commit_regime_bench_test.go | 24 ++- internal/impl/iceberg/config.go | 10 +- internal/impl/iceberg/parquet_compression.go | 125 +++++++---- .../impl/iceberg/parquet_compression_test.go | 200 ++++++++++++++++-- internal/impl/iceberg/router.go | 46 +++- internal/impl/iceberg/shredder/shredder.go | 97 ++++++--- .../iceberg/shredder/shredder_bench_test.go | 22 ++ .../shredder/shredder_paths_agree_test.go | 152 ++++++++++++- 10 files changed, 573 insertions(+), 119 deletions(-) diff --git a/docs/benchmark-results/iceberg.md b/docs/benchmark-results/iceberg.md index 81315a4bfe..e834fdc948 100644 --- a/docs/benchmark-results/iceberg.md +++ b/docs/benchmark-results/iceberg.md @@ -242,12 +242,14 @@ To reproduce: `GOMAXPROCS=1 go test -bench BenchmarkShredWide -benchmem -run '^$ --- -## Commit Regime — Commit Latency vs `max_in_flight` (synthetic) +## Commit Regime — Commit Latency vs `max_in_flight` (synthetic) — 2026-08-18 How commit coalescing responds to catalog commit latency and the number of concurrent in-flight submissions, measured by the flag-gated `TestCommitRegimeSweep` in [`internal/impl/iceberg/commit_regime_bench_test.go`](../../internal/impl/iceberg/commit_regime_bench_test.go). **Environment:** darwin/arm64, Apple M3 Pro; in-memory catalog with a fixed injected per-commit delay; 6s window per point; 300 records per submission +**Changed since last run:** first run of this harness ([#4712](https://github.com/redpanda-data/connect/pull/4712)). No production change — the committer and its batcher are as on `main`. These numbers describe batcher coalescing behaviour, so re-run them if the commit batching path changes. + **Caveat — read the numbers as ratios, not throughput.** Nothing here writes parquet or touches object storage, and the injected delay is not a real catalog, so the absolute rec/sec are not sink throughput figures and are not comparable with the localhost or live-catalog sections above. What the harness measures is how many submissions a commit carries, and at what latency. | commit latency | `max_in_flight` | rec/sec | records/commit | submissions/commit | diff --git a/docs/modules/components/pages/outputs/iceberg.adoc b/docs/modules/components/pages/outputs/iceberg.adoc index 6597c17ecd..25120595dc 100644 --- a/docs/modules/components/pages/outputs/iceberg.adoc +++ b/docs/modules/components/pages/outputs/iceberg.adoc @@ -310,11 +310,13 @@ Compression of the parquet data files this output writes is resolved per table, `parquet.compression` is optional rather than defaulted precisely so that step 2 is reachable: an unset field means "whatever the table says", which is not the same as explicitly choosing `uncompressed`. -*Prefer the table property where you can.* Mutations written with `merge_strategy: copy-on-write` rewrite whole data files, and that rewrite happens inside the Iceberg library rather than in this output's own writer — so `parquet.compression` does not reach it, but the table property does (the library reads the same property, and defaults it to `zstd` when absent). Setting the property therefore makes every file this output writes agree with itself; setting the field governs appends and merge-on-read writes only. Reach for the field when the property is not available to you — notably on catalogs that reject client-set table properties, such as the Databricks Unity Catalog. +*Prefer the table property where you can.* Several kinds of file are written by the Iceberg library rather than by this output's own writer, and `parquet.compression` cannot reach those: the whole-file rewrites performed by `merge_strategy: copy-on-write`, and the equality-delete files written by `merge-on-read`. The table property does reach them, because the library reads the same property (and defaults it to `zstd` when absent). So the property is the lever that gets every file in the table on the same codec, whereas the field governs only the data files this output writes itself — appends and merge-on-read data files. Reach for the field when the property is not available to you, notably on catalogs that reject client-set table properties, such as the Databricks Unity Catalog. -NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: appended files are uncompressed, while any copy-on-write rewrites are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data. +NOTE: Use a lower-case codec name in the property. This output accepts any casing, but the Iceberg library's own lookup is lower-case only and silently falls back to uncompressed for anything else — so a property of `ZSTD` would give you compressed appends and uncompressed rewrites. -*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, while `brotli` and `lzo` have patchy engine support. If the table property names one of those, it is reported at startup and files are written uncompressed rather than risking data an engine refuses to read. +NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: files this output writes are uncompressed, while any copy-on-write rewrites and equality-delete files are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data. + +*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this. *Cost.* Compression trades CPU for bytes, and this output is per-record CPU bound at low core counts, so enabling it on a small deployment can cost throughput. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most. @@ -1232,9 +1234,9 @@ Options: The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed. -Setting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property makes every file this output writes agree, whereas this field only governs appends and merge-on-read writes. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties. +Setting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties. -Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `brotli`, `lzo`), it is reported at startup and data files are written uncompressed rather than risking files a reader cannot open. +Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead. Compression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat. diff --git a/internal/impl/iceberg/commit_regime_bench_test.go b/internal/impl/iceberg/commit_regime_bench_test.go index d11e61650a..c3bbeb36aa 100644 --- a/internal/impl/iceberg/commit_regime_bench_test.go +++ b/internal/impl/iceberg/commit_regime_bench_test.go @@ -74,8 +74,13 @@ func (c *latentCatalog) CommitTable(ctx context.Context, ident table.Identifier, return nil, "", ctx.Err() } } - c.commits.Add(1) - return c.memCatalog.CommitTable(ctx, ident, reqs, updates) + meta, loc, err := c.memCatalog.CommitTable(ctx, ident, reqs, updates) + if err == nil { + // Counted after the fact so this is commits, not attempts: a retry would + // otherwise inflate the count and deflate the reported records/commit. + c.commits.Add(1) + } + return meta, loc, err } // regimeParams describes one point in the sweep. @@ -135,6 +140,10 @@ func runRegime(tb testing.TB, p regimeParams) regimeResult { start := time.Now() for i := range p.inFlight { wg.Go(func() { + // The deadline is checked before submitting, so elapsed includes the + // tail of each submitter's last in-flight commit and can overshoot + // the window by up to one commit latency. Records from that tail are + // counted too, so the derived rate stays representative. for time.Now().Before(deadline) { df, err := recordCountDataFile(tbl.Spec(), fmt.Sprintf("%s/data/%s.parquet", tbl.Location(), uuid.New()), @@ -225,7 +234,6 @@ func TestCommitRegimeSweep(t *testing.T) { window: window, }) results = append(results, res) - t.Log(res.String()) } } @@ -286,9 +294,13 @@ func TestCommitCoalescesConcurrentSubmissions(t *testing.T) { commits := cat.commits.Load() require.Positive(t, commits, "expected at least one commit") - require.Less(t, commits, int64(inFlight), - "expected %d concurrent submissions to coalesce into fewer than %d commits, got %d", - inFlight, inFlight, commits) + // Tight on purpose: `< inFlight` would pass at 7-of-8, which is essentially + // no coalescing. The mechanism under test merges a queued cohort into one + // commit, so allow only a small margin for the cohort being split across two + // commits by scheduling. + require.LessOrEqual(t, commits, int64(2), + "expected %d concurrent submissions to coalesce into at most 2 commits, got %d", + inFlight, commits) t.Logf("%d concurrent submissions coalesced into %d commits (%.2f submissions/commit)", inFlight, commits, float64(inFlight)/float64(commits)) } diff --git a/internal/impl/iceberg/config.go b/internal/impl/iceberg/config.go index 98982ee99d..8ffc2aa680 100644 --- a/internal/impl/iceberg/config.go +++ b/internal/impl/iceberg/config.go @@ -118,11 +118,13 @@ const compressionDocs = "\n" + "\n" + "`parquet.compression` is optional rather than defaulted precisely so that step 2 is reachable: an unset field means \"whatever the table says\", which is not the same as explicitly choosing `uncompressed`.\n" + "\n" + - "*Prefer the table property where you can.* Mutations written with `merge_strategy: copy-on-write` rewrite whole data files, and that rewrite happens inside the Iceberg library rather than in this output's own writer — so `parquet.compression` does not reach it, but the table property does (the library reads the same property, and defaults it to `zstd` when absent). Setting the property therefore makes every file this output writes agree with itself; setting the field governs appends and merge-on-read writes only. Reach for the field when the property is not available to you — notably on catalogs that reject client-set table properties, such as the Databricks Unity Catalog.\n" + + "*Prefer the table property where you can.* Several kinds of file are written by the Iceberg library rather than by this output's own writer, and `parquet.compression` cannot reach those: the whole-file rewrites performed by `merge_strategy: copy-on-write`, and the equality-delete files written by `merge-on-read`. The table property does reach them, because the library reads the same property (and defaults it to `zstd` when absent). So the property is the lever that gets every file in the table on the same codec, whereas the field governs only the data files this output writes itself — appends and merge-on-read data files. Reach for the field when the property is not available to you, notably on catalogs that reject client-set table properties, such as the Databricks Unity Catalog.\n" + "\n" + - "NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: appended files are uncompressed, while any copy-on-write rewrites are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data.\n" + + "NOTE: Use a lower-case codec name in the property. This output accepts any casing, but the Iceberg library's own lookup is lower-case only and silently falls back to uncompressed for anything else — so a property of `ZSTD` would give you compressed appends and uncompressed rewrites.\n" + "\n" + - "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, while `brotli` and `lzo` have patchy engine support. If the table property names one of those, it is reported at startup and files are written uncompressed rather than risking data an engine refuses to read.\n" + + "NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: files this output writes are uncompressed, while any copy-on-write rewrites and equality-delete files are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data.\n" + + "\n" + + "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this.\n" + "\n" + "*Cost.* Compression trades CPU for bytes, and this output is per-record CPU bound at low core counts, so enabling it on a small deployment can cost throughput. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most.\n" @@ -524,7 +526,7 @@ array:list Default("delta_length_byte_array"), service.NewStringEnumField(ioFieldParquetCompression, "uncompressed", "snappy", "gzip", "zstd"). - Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property makes every file this output writes agree, whereas this field only governs appends and merge-on-read writes. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `brotli`, `lzo`), it is reported at startup and data files are written uncompressed rather than risking files a reader cannot open.\n\nCompression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat."). + Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nCompression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat."). ShortDescription("Compression codec for written data files. Defaults to the table's write.parquet.compression-codec property, else uncompressed."). Optional(), ).Description("Parquet writer configuration."). diff --git a/internal/impl/iceberg/parquet_compression.go b/internal/impl/iceberg/parquet_compression.go index 1e3716b9ff..46606c4861 100644 --- a/internal/impl/iceberg/parquet_compression.go +++ b/internal/impl/iceberg/parquet_compression.go @@ -9,28 +9,30 @@ package iceberg import ( + "fmt" + "strings" + "github.com/apache/iceberg-go" "github.com/apache/iceberg-go/table" "github.com/parquet-go/parquet-go" "github.com/parquet-go/parquet-go/compress" - - "github.com/redpanda-data/benthos/v4/public/service" ) -// Compression codec names accepted by the `parquet.compression` field. These -// are deliberately a subset of the codecs parquet permits: every one of them is -// read by all the query engines this output targets. The codecs left out are -// interoperability hazards rather than technical gaps — +// Compression codec names this output will write. Deliberately a subset of what +// parquet permits and Iceberg's table property accepts: each of these is read by +// every query engine this output targets. The omissions are interoperability +// judgements rather than technical gaps — parquet-go can write brotli and +// LZ4_RAW, and declining them is a choice: // // - lz4: the original LZ4 codec was ambiguously specified (Hadoop framing vs. -// raw blocks) and readers disagree on which one `LZ4` means, which is why -// LZ4_RAW was later added to the format. Writing either risks files a given -// engine refuses. +// raw blocks) and readers disagree on which one `LZ4` means. parquet-go +// cannot write it at all. +// - lz4raw: the unambiguous replacement, so the objection above does not +// apply — but reader support for it is younger and less universal than +// snappy or zstd, which is reason enough not to write it by default. // - brotli, lzo: read support across engines is patchy. // -// A table property may still name one of those (see resolveParquetCompression), -// in which case this output declines to write it rather than produce files some -// reader cannot open. +// A table property may still name any of these; see resolveParquetCompression. const ( compressionUncompressed = "uncompressed" compressionSnappy = "snappy" @@ -39,8 +41,8 @@ const ( ) // parquetCompressionCodecs maps an accepted codec name to its parquet-go codec. -// "none" is included because that is the spelling Iceberg's own table property -// uses for no compression, and the property is a valid source of this value. +// "none" is included because that is the spelling Iceberg's table property uses +// for no compression, and the property is a valid source of this value. var parquetCompressionCodecs = map[string]compress.Codec{ compressionUncompressed: &parquet.Uncompressed, "none": &parquet.Uncompressed, @@ -49,51 +51,84 @@ var parquetCompressionCodecs = map[string]compress.Codec{ compressionZstd: &parquet.Zstd, } +// declinedCompressionCodecs are codec names parquet defines and Iceberg's table +// property accepts, but that this output will not write. Kept distinct from +// values that are simply unrecognised, so the operator is told which of the two +// happened. +var declinedCompressionCodecs = map[string]struct{}{ + "lz4": {}, + "lz4raw": {}, + "brotli": {}, + "lzo": {}, +} + +// normaliseCodecName folds a codec name for lookup. Codec names are ASCII, so +// lower-casing is sufficient. +// +// Both the configured value and the table property need this. The property +// obviously does — it is set by whoever owns the table, so its casing is not +// this output's to dictate. The configured value needs it too, and less +// obviously: the config framework's enum linter lower-cases before comparing +// against the option set, so `compression: ZSTD` passes validation and arrives +// here verbatim. Without folding it would miss the map and silently write +// uncompressed. +func normaliseCodecName(s string) string { + return strings.ToLower(strings.TrimSpace(s)) +} + // resolveParquetCompression decides which compression codec this output writes // data files with for one table, resolving in a fixed order: // // 1. `parquet.compression`, when set — an explicit operator instruction wins. -// 2. the table's own `write.parquet.compression-codec` property, so a table -// configured by its owner (or another writer) is honoured without needing -// connector configuration. Also the only way to make the copy-on-write -// rewrite path agree, since that path is inside iceberg-go and reads this -// property itself. -// 3. uncompressed, preserving this output's historical behaviour when neither -// is specified. +// 2. otherwise the table's own `write.parquet.compression-codec` property, so a +// table configured by its owner (or another writer) is honoured without +// needing connector configuration. +// 3. otherwise uncompressed, preserving this output's historical behaviour. // -// A property naming a codec this output declines to write (see the constants -// above) is reported and treated as unset rather than failing the write: the -// property is not this output's configuration to validate, and uncompressed is -// readable everywhere, so refusing to start would be a worse outcome than -// writing data every engine can read. An invalid *configured* value cannot -// reach here — the config field is an enum, validated at startup. +// A property naming a codec this output declines is treated as unset rather than +// failing the write: the property is not this output's configuration to +// validate, and uncompressed is readable everywhere, so refusing to start would +// be a worse outcome than writing data every engine can read. // -// Called once per table when its writer is built, not per batch. -func resolveParquetCompression(configured string, props iceberg.Properties, logger *service.Logger) compress.Codec { +// Returns the codec and, when something needs saying, a warning for the caller +// to log. The warning is returned rather than logged here so the caller can +// suppress repeats — writers are rebuilt on every write failure, so logging +// directly would spam a retrying pipeline. +func resolveParquetCompression(configured string, props iceberg.Properties) (codec compress.Codec, warning string) { if configured != "" { - if codec, ok := parquetCompressionCodecs[configured]; ok { - return codec + if codec, ok := parquetCompressionCodecs[normaliseCodecName(configured)]; ok { + return codec, "" } - // Unreachable via config validation; be explicit rather than silently - // writing something the operator did not ask for. - if logger != nil { - logger.Warnf("Unsupported %s.%s value %q; writing uncompressed data files.", ioFieldParquet, ioFieldParquetCompression, configured) - } - return &parquet.Uncompressed + // Reachable: the enum linter folds case before validating, so a + // differently-spelled-but-valid value passes config validation. A + // genuinely invalid value is rejected before it gets here. + return &parquet.Uncompressed, fmt.Sprintf( + "Unsupported %s.%s value %q; writing uncompressed data files. Supported values are uncompressed, snappy, gzip and zstd.", + ioFieldParquet, ioFieldParquetCompression, configured) } - fromTable, ok := props[table.ParquetCompressionKey] - if !ok || fromTable == "" { - return &parquet.Uncompressed + fromTable := normaliseCodecName(props[table.ParquetCompressionKey]) + if fromTable == "" { + return &parquet.Uncompressed, "" } if codec, ok := parquetCompressionCodecs[fromTable]; ok { - return codec + return codec, "" } - if logger != nil { - logger.Warnf("Table property %s is %q, which this output does not write (readers disagree on it or engine support is patchy); writing uncompressed data files instead. Set %s.%s to choose a supported codec explicitly.", - table.ParquetCompressionKey, fromTable, ioFieldParquet, ioFieldParquetCompression) + raw := props[table.ParquetCompressionKey] + if _, declined := declinedCompressionCodecs[fromTable]; declined { + // Deliberately does not claim the table is now safe: this governs only + // the files THIS writer produces. Copy-on-write rewrites and + // equality-delete files are written inside iceberg-go, which honours the + // same property and does map these codecs, so a table carrying one can + // still gain files in it. + return &parquet.Uncompressed, fmt.Sprintf( + "Table property %s is %q, which this output does not write: engine read support for it is not universal. The data files this output writes will be uncompressed instead — set %s.%s to choose a codec for them. Other writers, including this output's own copy-on-write rewrites and equality-delete files, may still honour the property, so change the property itself if nothing should write that codec.", + table.ParquetCompressionKey, raw, ioFieldParquet, ioFieldParquetCompression) } - return &parquet.Uncompressed + + return &parquet.Uncompressed, fmt.Sprintf( + "Table property %s is %q, which is not a compression codec this output recognises; writing uncompressed data files. Supported values are uncompressed, snappy, gzip and zstd.", + table.ParquetCompressionKey, raw) } diff --git a/internal/impl/iceberg/parquet_compression_test.go b/internal/impl/iceberg/parquet_compression_test.go index 34b0d1806c..06dcdc7172 100644 --- a/internal/impl/iceberg/parquet_compression_test.go +++ b/internal/impl/iceberg/parquet_compression_test.go @@ -26,13 +26,12 @@ import ( // TestResolveParquetCompression pins the resolution order: configured value, // then the table's own property, then uncompressed. func TestResolveParquetCompression(t *testing.T) { - logger := service.MockResources().Logger() - tests := []struct { - name string - configured string - props iceberg.Properties - want format.CompressionCodec + name string + configured string + props iceberg.Properties + want format.CompressionCodec + wantWarning string // substring; empty means no warning expected }{ { name: "nothing set writes uncompressed", @@ -48,6 +47,19 @@ func TestResolveParquetCompression(t *testing.T) { props: iceberg.Properties{table.ParquetCompressionKey: "snappy"}, want: format.Snappy, }, + { + // The config enum linter folds case before validating, so an + // upper-case value passes validation and arrives here verbatim. It + // must be honoured, not silently written uncompressed. + name: "upper-case configured value is honoured", + configured: "ZSTD", + want: format.Zstd, + }, + { + name: "mixed-case configured value is honoured", + configured: "Snappy", + want: format.Snappy, + }, { name: "configured value beats the table property", configured: compressionSnappy, @@ -75,25 +87,61 @@ func TestResolveParquetCompression(t *testing.T) { // The codecs deliberately not offered: honouring them would risk // files some reader refuses, so they degrade to uncompressed rather // than failing the write. - name: "lz4 from the table property degrades to uncompressed", - props: iceberg.Properties{table.ParquetCompressionKey: "lz4"}, - want: format.Uncompressed, + name: "lz4 from the table property degrades to uncompressed", + wantWarning: "does not write", + props: iceberg.Properties{table.ParquetCompressionKey: "lz4"}, + want: format.Uncompressed, }, { - name: "brotli from the table property degrades to uncompressed", - props: iceberg.Properties{table.ParquetCompressionKey: "brotli"}, - want: format.Uncompressed, + name: "brotli from the table property degrades to uncompressed", + wantWarning: "does not write", + props: iceberg.Properties{table.ParquetCompressionKey: "brotli"}, + want: format.Uncompressed, + }, + { + // The property belongs to whoever owns the table, so its casing is + // not this output's to dictate — "ZSTD" plainly means zstd. + name: "upper-case table property value is honoured", + props: iceberg.Properties{table.ParquetCompressionKey: "ZSTD"}, + want: format.Zstd, + }, + { + name: "mixed-case table property value is honoured", + props: iceberg.Properties{table.ParquetCompressionKey: "Snappy"}, + want: format.Snappy, }, { - name: "unknown table property value degrades to uncompressed", - props: iceberg.Properties{table.ParquetCompressionKey: "nonsense"}, + name: "upper-case NONE is honoured", + props: iceberg.Properties{table.ParquetCompressionKey: "NONE"}, want: format.Uncompressed, }, + { + name: "surrounding whitespace is tolerated", + props: iceberg.Properties{table.ParquetCompressionKey: " gzip "}, + want: format.Gzip, + }, + { + name: "a declined codec is declined regardless of casing", + wantWarning: "does not write", + props: iceberg.Properties{table.ParquetCompressionKey: "LZ4"}, + want: format.Uncompressed, + }, + { + name: "unknown table property value degrades to uncompressed", + wantWarning: "not a compression codec", + props: iceberg.Properties{table.ParquetCompressionKey: "nonsense"}, + want: format.Uncompressed, + }, { name: "empty table property value is treated as unset", props: iceberg.Properties{table.ParquetCompressionKey: ""}, want: format.Uncompressed, }, + { + name: "whitespace-only table property value is treated as unset", + props: iceberg.Properties{table.ParquetCompressionKey: " "}, + want: format.Uncompressed, + }, { name: "an unrelated property does not interfere", props: iceberg.Properties{"write.metadata.compression-codec": "gzip"}, @@ -103,8 +151,13 @@ func TestResolveParquetCompression(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := resolveParquetCompression(tc.configured, tc.props, logger) + got, warning := resolveParquetCompression(tc.configured, tc.props) require.Equal(t, tc.want, got.CompressionCodec()) + if tc.wantWarning == "" { + require.Empty(t, warning, "did not expect a warning") + } else { + require.Contains(t, warning, tc.wantWarning) + } }) } } @@ -132,7 +185,7 @@ func TestParquetCompressionReachesWrittenFile(t *testing.T) { pqSchema, fieldToCol, err := icebergx.BuildParquetSchema(sc, icebergx.TimestampEncoding(0)) require.NoError(t, err) - codec := resolveParquetCompression(tc.configured, tc.props, service.MockResources().Logger()) + codec, _ := resolveParquetCompression(tc.configured, tc.props) sink := newParquetSink(pqSchema, fieldToCol, true, parquet.Compression(codec)) // Enough rows of repetitive data that a codec has something to bite @@ -155,3 +208,118 @@ func TestParquetCompressionReachesWrittenFile(t *testing.T) { }) } } + +// TestWriterOptsForIsolatesTables pins the reason writerOptsFor uses +// slices.Concat rather than append: two tables resolving to different codecs +// must each get their own option, not share a backing array. +// +// The base slice is deliberately given spare capacity, because that is the only +// condition under which the append version of this bug bites — with a full +// slice, append would allocate and the aliasing would be invisible. +func TestWriterOptsForIsolatesTables(t *testing.T) { + base := make([]parquet.WriterOption, 0, 8) + base = append(base, parquet.DefaultEncodingFor(parquet.ByteArray, &parquet.Plain)) + + r := &Router{writerOpts: base, logger: service.MockResources().Logger()} + + zstdOpts := r.writerOptsFor(iceberg.Properties{table.ParquetCompressionKey: "zstd"}) + snappyOpts := r.writerOptsFor(iceberg.Properties{table.ParquetCompressionKey: "snappy"}) + + // Both carry the base option plus their own codec. + require.Len(t, zstdOpts, 2) + require.Len(t, snappyOpts, 2) + + // Resolving the second table must not have rewritten the first table's + // option. Compare the codecs the options actually apply, by configuring a + // writer with each and reading back what it would use. + require.Equal(t, format.Zstd, codecFromOptions(t, zstdOpts)) + require.Equal(t, format.Snappy, codecFromOptions(t, snappyOpts)) + + // The output-level slice itself must be untouched. + require.Len(t, r.writerOpts, 1) +} + +// codecFromOptions reports the compression codec a set of writer options +// resolves to, by applying them to a writer config. +func codecFromOptions(t *testing.T, opts []parquet.WriterOption) format.CompressionCodec { + t.Helper() + cfg, err := parquet.NewWriterConfig(opts...) + require.NoError(t, err) + require.NotNil(t, cfg.Compression, "options did not set a compression codec") + return cfg.Compression.CompressionCodec() +} + +// minimalIcebergYAML is the smallest config this output accepts, so the tests +// below can append only the parquet block they care about. +const minimalIcebergYAML = ` +catalog: + url: http://localhost:8181/api/catalog +namespace: ns +table: t +storage: + aws_s3: + bucket: bucket +` + +// TestParquetCompressionConfigParsing pins the design claim that an unset +// `parquet.compression` is NOT the same as an explicit "uncompressed": unset +// must leave the router's value empty so the table property remains reachable, +// while an explicit value must be carried through even when it happens to name +// the same codec as the fallback. +func TestParquetCompressionConfigParsing(t *testing.T) { + tests := []struct { + name string + conf string + want string + }{ + { + name: "no parquet block at all leaves it unset", + conf: "", + want: "", + }, + { + name: "a parquet block without compression leaves it unset", + conf: "parquet:\n string_encoding: plain\n", + want: "", + }, + { + name: "an explicit uncompressed is not the same as unset", + conf: "parquet:\n compression: uncompressed\n", + want: compressionUncompressed, + }, + { + name: "an explicit codec is carried through", + conf: "parquet:\n compression: zstd\n", + want: compressionZstd, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + conf, err := icebergOutputConfig().ParseYAML(minimalIcebergYAML+tc.conf, nil) + require.NoError(t, err) + + var got string + if conf.Contains(ioFieldParquet, ioFieldParquetCompression) { + got, err = conf.FieldString(ioFieldParquet, ioFieldParquetCompression) + require.NoError(t, err) + } + require.Equal(t, tc.want, got) + + // And the resolved codec follows from it: unset defers to the table + // property, explicit does not. + codec, _ := resolveParquetCompression(got, + iceberg.Properties{table.ParquetCompressionKey: "snappy"}) + // Unset falls through to the property (snappy); anything explicit + // wins over it. + wantCodec := format.Snappy + switch tc.want { + case compressionUncompressed: + wantCodec = format.Uncompressed + case compressionZstd: + wantCodec = format.Zstd + } + require.Equal(t, wantCodec, codec.CompressionCodec()) + }) + } +} diff --git a/internal/impl/iceberg/router.go b/internal/impl/iceberg/router.go index 5e62981b64..6cfc7bf4f6 100644 --- a/internal/impl/iceberg/router.go +++ b/internal/impl/iceberg/router.go @@ -147,6 +147,10 @@ type Router struct { entries sync.Map // tableKey -> *tableEntry + // warnedCompression de-duplicates compression warnings, keyed by warning + // text, so rebuilding a writer does not re-log one. See warnCompressionOnce. + warnedCompression sync.Map + // parquetCompression is the configured `parquet.compression` value, or "" // when unset. Set after construction by the output, like metrics below, so // NewRouter's signature stays put. Empty means each table falls back to its @@ -776,6 +780,38 @@ func (*Router) closeWriter(entry *tableEntry) { } } +// writerOptsFor returns the parquet writer options for one table: the options +// configured on the output, plus that table's resolved compression codec. +// +// Compression is resolved per table because the fallback reads the table's own +// write.parquet.compression-codec property, so two tables under one output can +// legitimately land on different codecs. +// +// slices.Concat, not append: appending to r.writerOpts would hand successive +// tables the same backing array whenever it has spare capacity, and each would +// overwrite the previous table's codec option. Pinned by +// TestWriterOptsForIsolatesTables. +func (r *Router) writerOptsFor(props iceberg.Properties) []parquet.WriterOption { + codec, warning := resolveParquetCompression(r.parquetCompression, props) + if warning != "" { + r.warnCompressionOnce(warning) + } + return slices.Concat(r.writerOpts, []parquet.WriterOption{parquet.Compression(codec)}) +} + +// warnCompressionOnce logs a compression warning the first time it is seen and +// stays quiet afterwards. Writers are rebuilt whenever a write fails (see +// closeWriter), so a retrying pipeline against a table whose property names an +// unwritable codec would otherwise emit the same warning without bound. +func (r *Router) warnCompressionOnce(warning string) { + if _, seen := r.warnedCompression.LoadOrStore(warning, struct{}{}); seen { + return + } + if r.logger != nil { + r.logger.Warn(warning) + } +} + // createWriter creates a new writer for a table. // Caller must hold entry.mu.Lock() and ensure entry.writer is nil. func (r *Router) createWriter(ctx context.Context, key tableKey, entry *tableEntry) (*writer, error) { @@ -919,15 +955,7 @@ func (r *Router) createWriter(ctx context.Context, key tableKey, entry *tableEnt } } - // Resolve compression per table, since the fallback reads that table's own - // property. slices.Concat rather than append: appending to r.writerOpts - // would share its backing array between tables, so two tables resolving to - // different codecs could overwrite each other's option. - writerOpts := slices.Concat(r.writerOpts, []parquet.WriterOption{ - parquet.Compression(resolveParquetCompression(r.parquetCompression, writerTbl.Properties(), r.logger)), - }) - - w := NewWriter(writerTbl, comm, r.caseSensitive, writerOpts, r.resolver, r.schemaEvoCfg.RequireSchemaMetadata, r.rowOpCfg, entry.tsEncoding, r.logger) + w := NewWriter(writerTbl, comm, r.caseSensitive, r.writerOptsFor(writerTbl.Properties()), r.resolver, r.schemaEvoCfg.RequireSchemaMetadata, r.rowOpCfg, entry.tsEncoding, r.logger) w.metrics = r.metrics r.logger.Debugf("Created writer for table %s.%s", key.namespace, key.table) diff --git a/internal/impl/iceberg/shredder/shredder.go b/internal/impl/iceberg/shredder/shredder.go index 2034125572..9dc55d1174 100644 --- a/internal/impl/iceberg/shredder/shredder.go +++ b/internal/impl/iceberg/shredder/shredder.go @@ -87,6 +87,14 @@ type RecordShredder struct { // iceberg's recommended convention and with engines like Spark and Trino // in their default configurations. caseSensitive bool + // duplicateFieldNames records whether any struct in the schema repeats a + // field name. When it does, shredStructExact cannot use its matched-count + // shortcut: the count is per field, so two fields sharing a name inflate it + // and an unaccounted-for input key can be mistaken for a full match, which + // would silently drop an unknown-field notification and with it schema + // evolution of that column. Pinned by + // TestShredStructPathsAgreeWithDuplicateFieldNames. + duplicateFieldNames bool // fieldCommons optionally maps an iceberg field ID to the upstream // schema.Common describing the same field. When present, the leaf // value-conversion step uses Logical params (timestamp unit, @@ -115,10 +123,50 @@ type RecordShredder struct { // if false, matching is case-insensitive (and ambiguous case-only duplicates // in the input cause an error). func NewRecordShredder(schema *iceberg.Schema, caseSensitive bool) *RecordShredder { + rootFields := schema.Fields() return &RecordShredder{ - schema: schema, - rootFields: schema.Fields(), - caseSensitive: caseSensitive, + schema: schema, + rootFields: rootFields, + caseSensitive: caseSensitive, + duplicateFieldNames: hasDuplicateFieldNames(rootFields), + } +} + +// hasDuplicateFieldNames reports whether any struct in the schema tree repeats a +// field name. Iceberg rejects duplicate field *IDs* but not duplicate names, and +// nothing upstream of the shredder enforces name uniqueness in case-sensitive +// mode — so this has to be treated as possible rather than assumed away. +// +// Computed once per shredder because it only gates an optimisation +// (shredStructExact's unknown-field shortcut); the walk is over schema +// structure, not data, so it costs nothing per record. +func hasDuplicateFieldNames(fields []iceberg.NestedField) bool { + seen := make(map[string]struct{}, len(fields)) + for _, field := range fields { + if _, dup := seen[field.Name]; dup { + return true + } + seen[field.Name] = struct{}{} + if nestedHasDuplicateFieldNames(field.Type) { + return true + } + } + return false +} + +// nestedHasDuplicateFieldNames recurses into whatever structs a type contains, +// including through list element and map value types, since those reach +// shredStruct too. +func nestedHasDuplicateFieldNames(typ iceberg.Type) bool { + switch t := typ.(type) { + case *iceberg.StructType: + return hasDuplicateFieldNames(t.FieldList) + case *iceberg.ListType: + return nestedHasDuplicateFieldNames(t.Element) + case *iceberg.MapType: + return nestedHasDuplicateFieldNames(t.KeyType) || nestedHasDuplicateFieldNames(t.ValueType) + default: + return false } } @@ -163,23 +211,12 @@ func (rs *RecordShredder) shredStruct( return rs.shredStructExact(fields, value, path, repLevel, defLevel, maxRepLevel, sink) } - return rs.shredStructFolded(fields, value, path, repLevel, defLevel, maxRepLevel, sink) -} - -// shredStructFolded is the case-insensitive path: input keys are matched against -// schema field names by their folded (lowercased) form, which means several -// distinct input keys can collide on one schema field and that collision has to -// be reported rather than silently resolved. -// -// shredStructExact must stay behaviourally identical to this for input that -// happens to match exactly; TestShredStructPathsAgree pins that. -func (rs *RecordShredder) shredStructFolded( - fields []iceberg.NestedField, - value map[string]any, - path icebergx.Path, - repLevel, defLevel, maxRepLevel int, - sink Sink, -) error { + // The case-insensitive body stays inline here rather than in a sibling + // function: it runs once per struct per record, and extracting it measured + // ~5% slower on BenchmarkShredWideFolded for the extra call. Nothing needs + // it callable on its own — TestShredStructPathsAgree compares the two paths + // through the public Shred entry point, using a shredder of each kind. + // // Build an index of input keys by their match-key (the original key in // case-sensitive mode, or its lowercase form in case-insensitive mode). // In case-insensitive mode, multiple input keys may collide on the same @@ -307,13 +344,19 @@ func (rs *RecordShredder) shredStructExact( } } - // Detect unknown fields in input. Field names are unique within a struct, - // so each matched field claimed exactly one distinct input key: when the - // counts agree, every key is accounted for and there is nothing to report. - // That is the steady state once a schema has stabilised, and it makes the - // common case allocation-free. (A count above len(value) is impossible for - // a well-formed schema, and would simply fall through to the scan.) - if matchedKeys == len(value) { + // Detect unknown fields in input. When field names are unique within a + // struct, each matched field claimed exactly one distinct input key, so + // agreeing counts mean every key is accounted for and there is nothing to + // report — the steady state once a schema has stabilised, and what makes the + // common case allocation-free. (A count above len(value) is harmless: it + // falls through to the scan.) + // + // Duplicate field names break that reasoning, because matchedKeys counts + // fields rather than distinct keys: fields [a, a] against {"a":1,"b":2} + // reaches 2 == 2 while "b" is genuinely unknown. Iceberg permits duplicate + // names, so the shortcut is disabled for such schemas rather than assumed + // away. + if !rs.duplicateFieldNames && matchedKeys == len(value) { return nil } diff --git a/internal/impl/iceberg/shredder/shredder_bench_test.go b/internal/impl/iceberg/shredder/shredder_bench_test.go index 364f306011..b6865c07ae 100644 --- a/internal/impl/iceberg/shredder/shredder_bench_test.go +++ b/internal/impl/iceberg/shredder/shredder_bench_test.go @@ -67,3 +67,25 @@ func BenchmarkShred(b *testing.B) { } } } + +// BenchmarkShredCaseInsensitive measures the same per-record shred cost for a +// case-insensitive shredder — the path that does NOT get the direct-lookup +// treatment, and so is the one at risk of quietly paying for optimisations made +// for the case-sensitive default. +// +// It earns its place: extracting the case-insensitive body into its own function +// (briefly, to make the two paths separately callable) cost ~5% here for the +// extra call, which only this benchmark revealed. +func BenchmarkShredCaseInsensitive(b *testing.B) { + rs := NewRecordShredder(benchSchema(), false) + record := benchRecord() + sink := discardSink{} + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := rs.Shred(record, sink); err != nil { + b.Fatal(err) + } + } +} diff --git a/internal/impl/iceberg/shredder/shredder_paths_agree_test.go b/internal/impl/iceberg/shredder/shredder_paths_agree_test.go index c0fb31cbae..c37cc7b8ac 100644 --- a/internal/impl/iceberg/shredder/shredder_paths_agree_test.go +++ b/internal/impl/iceberg/shredder/shredder_paths_agree_test.go @@ -27,13 +27,13 @@ import ( // notification, or error. // // The comparison goes through the public Shred entry point with two shredders -// rather than calling the two helpers directly, because shredStruct dispatches -// on rs.caseSensitive on *every* recursion: a single case-sensitive shredder -// routes nested structs to shredStructExact no matter which helper was called -// at the top level, so calling the helpers directly would compare the exact -// path against itself below the root and silently pass on any nested +// rather than reaching for the case-sensitive branch directly, because +// shredStruct dispatches on rs.caseSensitive on *every* recursion: a single +// case-sensitive shredder routes nested structs to shredStructExact regardless +// of how the top level was entered, so driving one shredder would compare the +// exact path against itself below the root and silently pass on any nested // divergence. Two shredders makes one run exact all the way down and the other -// folded all the way down. +// take the case-insensitive body all the way down. // // That comparison is only legitimate for a case-unambiguous corpus, so every // schema field name and record key below is lower-case: folding then maps each @@ -79,6 +79,41 @@ func TestShredStructPathsAgree(t *testing.T) { deep, ) + // Structs are also reached through list elements and map values, and those + // recursions dispatch on case sensitivity exactly like the top level does. + listOfStruct := iceberg.NestedField{ + ID: 30, + Name: "items", + Type: &iceberg.ListType{ + ElementID: 31, + Element: &iceberg.StructType{FieldList: []iceberg.NestedField{ + {ID: 32, Name: "k", Type: iceberg.PrimitiveTypes.String, Required: true}, + {ID: 33, Name: "v", Type: iceberg.PrimitiveTypes.Int64, Required: false}, + }}, + ElementRequired: false, + }, + Required: false, + } + mapOfStruct := iceberg.NestedField{ + ID: 40, + Name: "byname", + Type: &iceberg.MapType{ + KeyID: 41, + KeyType: iceberg.PrimitiveTypes.String, + ValueID: 42, + ValueType: &iceberg.StructType{FieldList: []iceberg.NestedField{ + {ID: 43, Name: "n", Type: iceberg.PrimitiveTypes.Int64, Required: false}, + }}, + ValueRequired: false, + }, + Required: false, + } + nestedSchema := iceberg.NewSchema(3, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true}, + listOfStruct, + mapOfStruct, + ) + emptySchema := iceberg.NewSchema(2) cases := []struct { @@ -161,6 +196,42 @@ func TestShredStructPathsAgree(t *testing.T) { schema: schema, record: map[string]any{"id": nil}, }, + { + name: "struct inside a list", + schema: nestedSchema, + record: map[string]any{"id": int64(1), "items": []any{ + map[string]any{"k": "a", "v": int64(1)}, + map[string]any{"k": "b"}, + }}, + }, + { + name: "unknown key in a struct inside a list", + schema: nestedSchema, + record: map[string]any{"id": int64(1), "items": []any{ + map[string]any{"k": "a", "surprise": true}, + }}, + }, + { + name: "required field missing in a struct inside a list", + schema: nestedSchema, + record: map[string]any{"id": int64(1), "items": []any{ + map[string]any{"v": int64(1)}, + }}, + }, + { + name: "struct inside a map value", + schema: nestedSchema, + record: map[string]any{"id": int64(1), "byname": map[string]any{ + "x": map[string]any{"n": int64(3)}, + }}, + }, + { + name: "unknown key in a struct inside a map value", + schema: nestedSchema, + record: map[string]any{"id": int64(1), "byname": map[string]any{ + "x": map[string]any{"n": int64(3), "extra": "?"}, + }}, + }, { name: "empty record", schema: schema, @@ -189,6 +260,13 @@ func TestShredStructPathsAgree(t *testing.T) { if foldedErr != nil { require.EqualError(t, exactErr, foldedErr.Error(), "fast path must fail exactly as the general path does") + // Still compare what was emitted before the error: an identical + // error does not imply identical partial output, and a divergence + // there would otherwise pass silently. + require.Equal(t, foldedSink.values, exactSink.values, + "values emitted before the error must be identical") + require.Equal(t, sortedNewFields(foldedSink.newFields), sortedNewFields(exactSink.newFields), + "new-field notifications before the error must be identical") return } require.NoError(t, exactErr, "fast path errored where the general path did not") @@ -212,3 +290,65 @@ func sortedNewFields(in []newFieldRecord) []string { sort.Strings(out) return out } + +// TestShredStructPathsAgreeWithDuplicateFieldNames covers the one input class +// where the fast path's matched-count shortcut is unsound: Iceberg rejects +// duplicate field IDs but not duplicate field *names*, and matchedKeys counts +// fields rather than distinct claimed keys. Fields [a, a] against +// {"a":…, "b":…} therefore reach matchedKeys == len(value) while "b" is +// genuinely unknown, and without the duplicateFieldNames guard the fast path +// would skip the scan and never report it — losing the schema evolution of that +// column, silently. +// +// Kept separate from TestShredStructPathsAgree because the schema cannot be +// built with iceberg.NewSchema's validation in the same table as the others. +func TestShredStructPathsAgreeWithDuplicateFieldNames(t *testing.T) { + fields := []iceberg.NestedField{ + {ID: 1, Name: "a", Type: iceberg.PrimitiveTypes.Int64, Required: false}, + {ID: 2, Name: "a", Type: iceberg.PrimitiveTypes.Int64, Required: false}, + } + record := map[string]any{"a": int64(1), "b": int64(2)} + + exactSink := &testSink{} + exact := &RecordShredder{caseSensitive: true, duplicateFieldNames: hasDuplicateFieldNames(fields)} + require.NoError(t, exact.shredStruct(fields, record, nil, 0, 0, 0, exactSink)) + + foldedSink := &testSink{} + folded := &RecordShredder{caseSensitive: false} + require.NoError(t, folded.shredStruct(fields, record, nil, 0, 0, 0, foldedSink)) + + require.Equal(t, foldedSink.values, exactSink.values) + require.Equal(t, sortedNewFields(foldedSink.newFields), sortedNewFields(exactSink.newFields), + "the unknown key must be reported by both paths") + require.Len(t, exactSink.newFields, 1, "expected the unknown key to be reported") + require.Equal(t, "b", exactSink.newFields[0].name) +} + +// TestHasDuplicateFieldNames pins the detection itself, including through the +// list and map recursions. +func TestHasDuplicateFieldNames(t *testing.T) { + str := iceberg.PrimitiveTypes.String + dupStruct := &iceberg.StructType{FieldList: []iceberg.NestedField{ + {ID: 10, Name: "x", Type: str}, {ID: 11, Name: "x", Type: str}, + }} + + require.False(t, hasDuplicateFieldNames([]iceberg.NestedField{ + {ID: 1, Name: "a", Type: str}, {ID: 2, Name: "b", Type: str}, + }), "distinct names") + + require.True(t, hasDuplicateFieldNames([]iceberg.NestedField{ + {ID: 1, Name: "a", Type: str}, {ID: 2, Name: "a", Type: str}, + }), "top-level duplicate") + + require.True(t, hasDuplicateFieldNames([]iceberg.NestedField{ + {ID: 1, Name: "s", Type: dupStruct}, + }), "duplicate nested in a struct") + + require.True(t, hasDuplicateFieldNames([]iceberg.NestedField{ + {ID: 1, Name: "l", Type: &iceberg.ListType{ElementID: 2, Element: dupStruct}}, + }), "duplicate nested in a list element") + + require.True(t, hasDuplicateFieldNames([]iceberg.NestedField{ + {ID: 1, Name: "m", Type: &iceberg.MapType{KeyID: 2, KeyType: str, ValueID: 3, ValueType: dupStruct}}, + }), "duplicate nested in a map value") +} From e55a882fa686e065f7ff57ebfce76677061900fe Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Wed, 26 Aug 2026 10:30:21 +0100 Subject: [PATCH 08/12] iceberg: fix a stale benchmark reference and unmeasured perf claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review points. The comment justifying keeping the case-insensitive shredding body inline cited BenchmarkShredWideFolded, which never existed in the tree — it was the name of a throwaway probe used to take the measurement. The benchmark that now guards that path, and that records the same figure, is BenchmarkShredCaseInsensitive. Named correctly so the measurement behind the decision can actually be re-run. The compression field shipped a throughput claim with no measurement behind it. Reworded to separate what is measured from what is not: profiling at one vCPU does attribute roughly a seventh of this output's CPU to parquet encoding and does find it CPU bound per record at low core counts, but the cost of any particular codec has not been measured, so the docs now say that and point the reader at measuring their own workload instead of implying a known result. The end-to-end throughput re-run that docs/benchmarking.md asks for after a hot path change is still missing, and this records why rather than leaving it unexplained: the localhost suite drives the `iceberg` output, which is an enterprise component, and the license available here has expired, so the pipeline refuses to start and no before/after pair can be produced. The benchmark results file now states that and what to run once a current license is available — before/after plus one run per codec, at one and four cores. --- docs/benchmark-results/iceberg.md | 1 + docs/modules/components/pages/outputs/iceberg.adoc | 4 ++-- internal/impl/iceberg/config.go | 4 ++-- internal/impl/iceberg/shredder/shredder.go | 6 +++--- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/benchmark-results/iceberg.md b/docs/benchmark-results/iceberg.md index e834fdc948..60f22717c9 100644 --- a/docs/benchmark-results/iceberg.md +++ b/docs/benchmark-results/iceberg.md @@ -236,6 +236,7 @@ Per sub-benchmark, sec/op: `declared_schema=false` 4.304µs → 1.394µs (-67.6% **Observations:** - **This is the shredder in isolation, not a sink-level number.** Earlier 1-vCPU profiling attributed ~27% of the sink's CPU to shredding, so the end-to-end effect should be appreciable but much smaller than 66%. **It has not been measured end to end** — no throughput figure above or elsewhere in this file has been re-run for this change. +- **Why not:** the localhost suite runs the `iceberg` output, which is an enterprise component, and the license available while this work was done had expired — the pipeline refuses to start, so no before/after throughput pair could be produced. The same applies to quantifying what the new `parquet.compression` codecs cost per record. Both are outstanding: re-run `task bench` (before/after, and once per codec at one and four cores) against a current license and append the sections here. - The two `declared_schema` variants are within noise of each other both before and after, consistent with the earlier finding that the `schema_metadata` knob does not bypass decode, shredding or encode. To reproduce: `GOMAXPROCS=1 go test -bench BenchmarkShredWide -benchmem -run '^$' -count=8 ./internal/impl/iceberg/bench/` diff --git a/docs/modules/components/pages/outputs/iceberg.adoc b/docs/modules/components/pages/outputs/iceberg.adoc index 25120595dc..5d591125e0 100644 --- a/docs/modules/components/pages/outputs/iceberg.adoc +++ b/docs/modules/components/pages/outputs/iceberg.adoc @@ -318,7 +318,7 @@ NOTE: A consequence of the above is that a table left entirely unconfigured alre *Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this. -*Cost.* Compression trades CPU for bytes, and this output is per-record CPU bound at low core counts, so enabling it on a small deployment can cost throughput. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most. +*Cost.* Compression trades CPU for bytes. Profiling this output at one vCPU attributes roughly a seventh of its CPU to parquet encoding and finds it CPU bound per record at low core counts, so expect some throughput cost there — though the size of that cost has not yet been measured per codec, so treat it as a reason to measure your own workload rather than as a number. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most. == Performance @@ -1238,7 +1238,7 @@ Setting the table property rather than this field is usually the better choice, Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead. -Compression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat. +Compression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat. *Type*: `string` diff --git a/internal/impl/iceberg/config.go b/internal/impl/iceberg/config.go index 8ffc2aa680..3671411678 100644 --- a/internal/impl/iceberg/config.go +++ b/internal/impl/iceberg/config.go @@ -126,7 +126,7 @@ const compressionDocs = "\n" + "\n" + "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this.\n" + "\n" + - "*Cost.* Compression trades CPU for bytes, and this output is per-record CPU bound at low core counts, so enabling it on a small deployment can cost throughput. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most.\n" + "*Cost.* Compression trades CPU for bytes. Profiling this output at one vCPU attributes roughly a seventh of its CPU to parquet encoding and finds it CPU bound per record at low core counts, so expect some throughput cost there — though the size of that cost has not yet been measured per codec, so treat it as a reason to measure your own workload rather than as a number. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most.\n" // rowOperationDocs is the long-form documentation for the row-level operation // feature. It lives in the component description rather than inline in the @@ -526,7 +526,7 @@ array:list Default("delta_length_byte_array"), service.NewStringEnumField(ioFieldParquetCompression, "uncompressed", "snappy", "gzip", "zstd"). - Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nCompression trades CPU for size, and this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment. See <> for the full resolution order and the copy-on-write caveat."). + Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nCompression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat."). ShortDescription("Compression codec for written data files. Defaults to the table's write.parquet.compression-codec property, else uncompressed."). Optional(), ).Description("Parquet writer configuration."). diff --git a/internal/impl/iceberg/shredder/shredder.go b/internal/impl/iceberg/shredder/shredder.go index 9dc55d1174..0ca642dae1 100644 --- a/internal/impl/iceberg/shredder/shredder.go +++ b/internal/impl/iceberg/shredder/shredder.go @@ -213,9 +213,9 @@ func (rs *RecordShredder) shredStruct( // The case-insensitive body stays inline here rather than in a sibling // function: it runs once per struct per record, and extracting it measured - // ~5% slower on BenchmarkShredWideFolded for the extra call. Nothing needs - // it callable on its own — TestShredStructPathsAgree compares the two paths - // through the public Shred entry point, using a shredder of each kind. + // ~5% slower on BenchmarkShredCaseInsensitive for the extra call. Nothing + // needs it callable on its own — TestShredStructPathsAgree compares the two + // paths through the public Shred entry point, using a shredder of each kind. // // Build an index of input keys by their match-key (the original key in // case-sensitive mode, or its lowercase form in case-insensitive mode). From 0df23cd7cb8bb92e0b6b814a44adb7bf8e46e0a8 Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Wed, 26 Aug 2026 15:32:22 +0100 Subject: [PATCH 09/12] iceberg: name the table in compression warnings, accept lz4_raw spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two diagnostic fixes from review; neither changes what gets written. The compression warning never said which table it was about, and the de-duplication then keyed on the message text alone. A router is inherently multi-table — namespace and table are interpolated per message — so two tables carrying the same unwritable property value produced exactly one warning naming neither, and no way to tell whose data files had silently gone uncompressed. The warning now names the table and the de-duplication is keyed per table, which keeps the unbounded-retry suppression it exists for (writers are rebuilt on every write failure) while still reporting each affected table once. LZ4_RAW was only spelled the way parquet-go names its own option, "lz4raw". A table property follows the parquet codec names, where it is LZ4_RAW — the spelling this repo already records for it in the parquet processor's codec list — so a property of `lz4_raw` fell past the declined set and the operator was told the value "is not a compression codec this output recognises" instead of the accurate "this output does not write it". Both spellings are accepted now, since either can turn up depending on which writer set the property, and the docs use `lz4_raw` because that is what a user would actually type. Tests: the three LZ4_RAW spellings reach the declined branch rather than the unrecognised one, and a new test pins the warning being per table — three writer builds for one table log once, a second table with the identical property value is still reported, and a supported codec stays silent. --- .../components/pages/outputs/iceberg.adoc | 4 +- internal/impl/iceberg/config.go | 4 +- internal/impl/iceberg/parquet_compression.go | 16 +++++-- .../impl/iceberg/parquet_compression_test.go | 48 ++++++++++++++++++- internal/impl/iceberg/router.go | 22 +++++---- 5 files changed, 75 insertions(+), 19 deletions(-) diff --git a/docs/modules/components/pages/outputs/iceberg.adoc b/docs/modules/components/pages/outputs/iceberg.adoc index 5d591125e0..6196fd8df6 100644 --- a/docs/modules/components/pages/outputs/iceberg.adoc +++ b/docs/modules/components/pages/outputs/iceberg.adoc @@ -316,7 +316,7 @@ NOTE: Use a lower-case codec name in the property. This output accepts any casin NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: files this output writes are uncompressed, while any copy-on-write rewrites and equality-delete files are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data. -*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this. +*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4_raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this. *Cost.* Compression trades CPU for bytes. Profiling this output at one vCPU attributes roughly a seventh of its CPU to parquet encoding and finds it CPU bound per record at low core counts, so expect some throughput cost there — though the size of that cost has not yet been measured per codec, so treat it as a reason to measure your own workload rather than as a number. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most. @@ -1236,7 +1236,7 @@ The compression codec for data files this output writes. **Optional on purpose** Setting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties. -Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead. +Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4_raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead. Compression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat. diff --git a/internal/impl/iceberg/config.go b/internal/impl/iceberg/config.go index 3671411678..16f3c0a7d6 100644 --- a/internal/impl/iceberg/config.go +++ b/internal/impl/iceberg/config.go @@ -124,7 +124,7 @@ const compressionDocs = "\n" + "\n" + "NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: files this output writes are uncompressed, while any copy-on-write rewrites and equality-delete files are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data.\n" + "\n" + - "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this.\n" + + "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4_raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this.\n" + "\n" + "*Cost.* Compression trades CPU for bytes. Profiling this output at one vCPU attributes roughly a seventh of its CPU to parquet encoding and finds it CPU bound per record at low core counts, so expect some throughput cost there — though the size of that cost has not yet been measured per codec, so treat it as a reason to measure your own workload rather than as a number. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most.\n" @@ -526,7 +526,7 @@ array:list Default("delta_length_byte_array"), service.NewStringEnumField(ioFieldParquetCompression, "uncompressed", "snappy", "gzip", "zstd"). - Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nCompression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat."). + Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4_raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nCompression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat."). ShortDescription("Compression codec for written data files. Defaults to the table's write.parquet.compression-codec property, else uncompressed."). Optional(), ).Description("Parquet writer configuration."). diff --git a/internal/impl/iceberg/parquet_compression.go b/internal/impl/iceberg/parquet_compression.go index 46606c4861..56ce663a6d 100644 --- a/internal/impl/iceberg/parquet_compression.go +++ b/internal/impl/iceberg/parquet_compression.go @@ -27,7 +27,7 @@ import ( // - lz4: the original LZ4 codec was ambiguously specified (Hadoop framing vs. // raw blocks) and readers disagree on which one `LZ4` means. parquet-go // cannot write it at all. -// - lz4raw: the unambiguous replacement, so the objection above does not +// - lz4_raw: the unambiguous replacement, so the objection above does not // apply — but reader support for it is younger and less universal than // snappy or zstd, which is reason enough not to write it by default. // - brotli, lzo: read support across engines is patchy. @@ -56,10 +56,16 @@ var parquetCompressionCodecs = map[string]compress.Codec{ // values that are simply unrecognised, so the operator is told which of the two // happened. var declinedCompressionCodecs = map[string]struct{}{ - "lz4": {}, - "lz4raw": {}, - "brotli": {}, - "lzo": {}, + "lz4": {}, + // Both spellings: a table property follows the parquet codec names, where it + // is LZ4_RAW, but iceberg-go's own lookup spells it "lz4raw" — so either can + // turn up in a property depending on who set it. Getting both here is what + // makes the operator see "this output does not write it" rather than the + // misleading "not a codec this output recognises". + "lz4_raw": {}, + "lz4raw": {}, + "brotli": {}, + "lzo": {}, } // normaliseCodecName folds a codec name for lookup. Codec names are ASCII, so diff --git a/internal/impl/iceberg/parquet_compression_test.go b/internal/impl/iceberg/parquet_compression_test.go index 06dcdc7172..867abc8ee3 100644 --- a/internal/impl/iceberg/parquet_compression_test.go +++ b/internal/impl/iceberg/parquet_compression_test.go @@ -9,6 +9,9 @@ package iceberg import ( + "bytes" + "log/slog" + "strings" "testing" "github.com/apache/iceberg-go" @@ -222,8 +225,10 @@ func TestWriterOptsForIsolatesTables(t *testing.T) { r := &Router{writerOpts: base, logger: service.MockResources().Logger()} - zstdOpts := r.writerOptsFor(iceberg.Properties{table.ParquetCompressionKey: "zstd"}) - snappyOpts := r.writerOptsFor(iceberg.Properties{table.ParquetCompressionKey: "snappy"}) + zstdOpts := r.writerOptsFor(tableKey{namespace: "ns", table: "a"}, + iceberg.Properties{table.ParquetCompressionKey: "zstd"}) + snappyOpts := r.writerOptsFor(tableKey{namespace: "ns", table: "b"}, + iceberg.Properties{table.ParquetCompressionKey: "snappy"}) // Both carry the base option plus their own codec. require.Len(t, zstdOpts, 2) @@ -323,3 +328,42 @@ func TestParquetCompressionConfigParsing(t *testing.T) { }) } } + +// TestCompressionWarningIsPerTable pins two things about the warning that a +// router-wide de-duplication could plausibly get wrong: it must name the table +// it concerns, and it must not swallow a second affected table just because the +// message text repeats. +// +// It must still suppress repeats for the SAME table, which is the reason the +// de-duplication exists — writers are rebuilt on every write failure, so a +// retrying pipeline would otherwise log without bound. +func TestCompressionWarningIsPerTable(t *testing.T) { + var buf bytes.Buffer + logger := service.NewLoggerFromSlog(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{ + Level: slog.LevelWarn, + }))) + r := &Router{logger: logger} + + badProps := iceberg.Properties{table.ParquetCompressionKey: "lz4"} + first := tableKey{namespace: "ns", table: "orders"} + second := tableKey{namespace: "ns", table: "payments"} + + // Same table three times — as a retry loop rebuilding its writer would. + for range 3 { + r.writerOptsFor(first, badProps) + } + // A different table with the identical property value. + r.writerOptsFor(second, badProps) + + logged := buf.String() + require.Equal(t, 1, strings.Count(logged, "ns.orders"), + "expected exactly one warning for the first table despite three writer builds") + require.Equal(t, 1, strings.Count(logged, "ns.payments"), + "the second affected table must still be reported, not swallowed by de-duplication") + + // A table whose property is fine must stay silent. + buf.Reset() + r.writerOptsFor(tableKey{namespace: "ns", table: "fine"}, + iceberg.Properties{table.ParquetCompressionKey: "zstd"}) + require.Empty(t, buf.String(), "a supported codec must not warn") +} diff --git a/internal/impl/iceberg/router.go b/internal/impl/iceberg/router.go index 6cfc7bf4f6..447b19a87e 100644 --- a/internal/impl/iceberg/router.go +++ b/internal/impl/iceberg/router.go @@ -791,24 +791,30 @@ func (*Router) closeWriter(entry *tableEntry) { // tables the same backing array whenever it has spare capacity, and each would // overwrite the previous table's codec option. Pinned by // TestWriterOptsForIsolatesTables. -func (r *Router) writerOptsFor(props iceberg.Properties) []parquet.WriterOption { +func (r *Router) writerOptsFor(key tableKey, props iceberg.Properties) []parquet.WriterOption { codec, warning := resolveParquetCompression(r.parquetCompression, props) if warning != "" { - r.warnCompressionOnce(warning) + r.warnCompressionOnce(key, warning) } return slices.Concat(r.writerOpts, []parquet.WriterOption{parquet.Compression(codec)}) } -// warnCompressionOnce logs a compression warning the first time it is seen and -// stays quiet afterwards. Writers are rebuilt whenever a write fails (see +// warnCompressionOnce logs a compression warning once per table and stays quiet +// for that table afterwards. Writers are rebuilt whenever a write fails (see // closeWriter), so a retrying pipeline against a table whose property names an // unwritable codec would otherwise emit the same warning without bound. -func (r *Router) warnCompressionOnce(warning string) { - if _, seen := r.warnedCompression.LoadOrStore(warning, struct{}{}); seen { +// +// Keyed on the table as well as the message, because a router is inherently +// multi-table — namespace and table are interpolated per message — so keying on +// the message alone would report the first affected table and silently swallow +// every other one. The message names the table for the same reason: otherwise +// there is no way to tell whose data files went uncompressed. +func (r *Router) warnCompressionOnce(key tableKey, warning string) { + if _, seen := r.warnedCompression.LoadOrStore(key.namespace+"\x00"+key.table+"\x00"+warning, struct{}{}); seen { return } if r.logger != nil { - r.logger.Warn(warning) + r.logger.Warnf("Table %s.%s: %s", key.namespace, key.table, warning) } } @@ -955,7 +961,7 @@ func (r *Router) createWriter(ctx context.Context, key tableKey, entry *tableEnt } } - w := NewWriter(writerTbl, comm, r.caseSensitive, r.writerOptsFor(writerTbl.Properties()), r.resolver, r.schemaEvoCfg.RequireSchemaMetadata, r.rowOpCfg, entry.tsEncoding, r.logger) + w := NewWriter(writerTbl, comm, r.caseSensitive, r.writerOptsFor(key, writerTbl.Properties()), r.resolver, r.schemaEvoCfg.RequireSchemaMetadata, r.rowOpCfg, entry.tsEncoding, r.logger) w.metrics = r.metrics r.logger.Debugf("Created writer for table %s.%s", key.namespace, key.table) From e4547874fe5e5b200303ebf86d15754c426c678d Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Thu, 27 Aug 2026 13:32:10 +0100 Subject: [PATCH 10/12] iceberg: measure write-path throughput without the pipeline suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The end-to-end numbers this branch was missing, obtained a layer below the suite that could not produce them. The localhost pipeline benchmark runs the assembled `iceberg` output, which is an enterprise component and so needs a valid licence to initialise. The write path itself does not: `license.CheckRunningEnterprise` is called by the output constructor, not by the Router, so driving the Router against the containerised MinIO + Iceberg REST catalog — as the integration tests already do — measures JSON decode, shredding, parquet encode, upload and catalog commit with no licence involved. That is what TestWriteThroughput does. The harness refuses to report a rate it cannot vouch for. It asserts the table holds exactly the records written, reads the data back to confirm the columns are populated, and reads the codec out of a written file's footer and fails if it is not the one requested. Every one of those caught something: a run that reported 36,846 msg/s while exiting immediately on the licence error; a bytes-per-record figure taken from the snapshot summary's total-files-size, which this catalog reports as 82kB for a table whose string column alone reads back as 528kB; and a supposedly uncompressed run that was in fact writing zstd. Results are recorded in docs/benchmark-results/iceberg.md. Two of them contradict things this branch previously asserted, so the docs are corrected: Compression has no throughput cost worth planning around. Every codec landed within a few percent of uncompressed, in both directions, at one core as well as four. The claim that it would cost throughput at low core counts was reasoning, not measurement, and the measurement does not support it. The size effect meanwhile is entirely data-dependent — zstd was ~15x smaller than uncompressed on a repetitive record shape and ~2% smaller on random content. A table created through the Iceberg library, which includes tables this output creates itself, comes back carrying write.parquet.compression-codec: zstd, materialised at creation. Since an unset field defers to the property, such tables get zstd — so the uncompressed default applies only to a table whose property is genuinely absent, which is narrower than the docs implied. The shredder change shows no measurable end-to-end gain, at 5 or 50 columns and at one or four cores, against a 66% reduction in isolation. The likeliest reading is that this harness is not shredder-bound — per-record time is dominated by encode, upload and commit, and GOMAXPROCS=1 here constrains the writer while MinIO and the catalog have their own cores. Recorded as it stands rather than explained away: the isolated win is measured, its end-to-end value on these shapes is not demonstrated. Also adds a payload shape flag. The first attempt at high-entropy data sliced a 64kB pool 100k times, and parquet's encoders exploited the overlap well enough to look like 4:1 compression on supposedly random input, which would have made the codec comparison meaningless. Content is now generated per record. --- docs/benchmark-results/iceberg.md | 59 +++- .../components/pages/outputs/iceberg.adoc | 6 +- internal/impl/iceberg/config.go | 6 +- .../integration/throughput_bench_test.go | 302 ++++++++++++++++++ 4 files changed, 366 insertions(+), 7 deletions(-) create mode 100644 internal/impl/iceberg/integration/throughput_bench_test.go diff --git a/docs/benchmark-results/iceberg.md b/docs/benchmark-results/iceberg.md index 60f22717c9..cefba36e68 100644 --- a/docs/benchmark-results/iceberg.md +++ b/docs/benchmark-results/iceberg.md @@ -236,7 +236,7 @@ Per sub-benchmark, sec/op: `declared_schema=false` 4.304µs → 1.394µs (-67.6% **Observations:** - **This is the shredder in isolation, not a sink-level number.** Earlier 1-vCPU profiling attributed ~27% of the sink's CPU to shredding, so the end-to-end effect should be appreciable but much smaller than 66%. **It has not been measured end to end** — no throughput figure above or elsewhere in this file has been re-run for this change. -- **Why not:** the localhost suite runs the `iceberg` output, which is an enterprise component, and the license available while this work was done had expired — the pipeline refuses to start, so no before/after throughput pair could be produced. The same applies to quantifying what the new `parquet.compression` codecs cost per record. Both are outstanding: re-run `task bench` (before/after, and once per codec at one and four cores) against a current license and append the sections here. +- **Since measured at the write path** — see "Write-path Throughput" below, which drives the Router directly and so does not need the licence the full pipeline suite does. It found no measurable end-to-end gain from this change on the shapes tested. The full-pipeline figures under "Write Throughput" above are still not re-run: that suite runs the assembled enterprise output and needs a valid licence. - The two `declared_schema` variants are within noise of each other both before and after, consistent with the earlier finding that the `schema_metadata` knob does not bypass decode, shredding or encode. To reproduce: `GOMAXPROCS=1 go test -bench BenchmarkShredWide -benchmem -run '^$' -count=8 ./internal/impl/iceberg/bench/` @@ -278,6 +278,63 @@ To reproduce: `go test -run TestCommitRegimeSweep -iceberg.commit-regime -timeou --- +## Write-path Throughput — Shredder Change, End to End — 2026-08-27 + +The sink write path driven directly against containerised MinIO + Iceberg REST, by the flag-gated `TestWriteThroughput` in [`internal/impl/iceberg/integration/`](../../internal/impl/iceberg/integration/). Measures JSON decode, shredding, parquet encode, upload and catalog commit. Compression held at `uncompressed` so only the shredder differs. + +**Environment:** darwin/arm64, Apple M3 Pro; MinIO + `apache/iceberg-rest-fixture` in containers on the same machine; batches of 5,000 records; n=1 per point + +**Changed since last run:** the case-sensitive shredding path ([#4712](https://github.com/redpanda-data/connect/pull/4712)), measured against the same code with that change reverted. + +| schema | cores | records | before (rec/s) | after (rec/s) | +|---|---:|---:|---:|---:| +| 5 columns | 1 | 200,000 | 123,599 | 120,185 | +| 5 columns | 4 | 200,000 | 133,732 | 133,621 | +| 50 columns | 1 | 100,000 | 38,132 / 36,182 | 37,314 / 36,508 | + +**Observations:** + +- **No measurable end-to-end gain, at either schema width or core count.** The isolated shredder benchmark for this change is a 66% reduction (see the section above), and none of it shows up here. The 50-column runs were repeated twice per side precisely because the difference is inside run-to-run variance. +- **The most likely reading is that this harness is not shredder-bound.** Even at 50 columns, per-record time here is dominated by parquet encode, upload and commit, and the earlier profile that motivated the change (46% JSON decode, 27% shredding) came from a full pipeline run under the actual binary, not from this seam. +- **Treat `GOMAXPROCS=1` here as "one core for the writer", not as a 1-vCPU deployment.** MinIO and the catalog run in containers with their own cores on the same machine, and there is no benthos input or pipeline in the loop, so the CPU mix differs from a constrained container running the whole thing. +- Consequence for the shredder change: the isolated win is solid and measured, and its end-to-end value on these workloads is **not demonstrated**. It reduces per-record allocations and CPU in a component that profiling says accounts for about a quarter of sink CPU; whether that is visible at the sink depends on what else the workload is spending time on, and on these two record shapes it is not. + +To reproduce: `TESTCONTAINERS_RYUK_DISABLED=true go test ./internal/impl/iceberg/integration/ -run TestWriteThroughput -timeout 25m -iceberg.throughput -iceberg.throughput.records=200000 -iceberg.throughput.codec=uncompressed` (add `-iceberg.throughput.columns=45` for the wide schema). + +--- + +## Write-path Throughput — Compression Codecs — 2026-08-27 + +Same harness, varying the table's `write.parquet.compression-codec` property. 100,000 records per point, n=1. The harness reads the codec back out of a written file's footer and fails the run if it is not the one requested, so each row is a measurement of the codec named. + +**Environment:** as above + +**Changed since last run:** first measurement of the `parquet.compression` field's codecs ([#4712](https://github.com/redpanda-data/connect/pull/4712)). + +Record shape matters more than anything else here, so both are given. "regular" is ~90 B with sequential ids and an `info` string sharing a 21-character prefix; "high-entropy" fills `info` with 1,100 freshly random characters per record. + +| payload | codec | rec/s (4 cores) | rec/s (1 core) | bytes/record | +|---|---|---:|---:|---:| +| regular | uncompressed | 120,693 | 116,524 | 57.2 | +| regular | snappy | 128,934 | 114,261 | 14.7 | +| regular | zstd | 129,081 | 119,433 | 3.9 | +| high-entropy | uncompressed | 46,396 | 42,994 | 1,151.4 | +| high-entropy | snappy | 46,089 | — | 1,130.8 | +| high-entropy | zstd | 45,152 | 43,209 | 1,124.4 | + +**Observations:** + +- **Compression showed no throughput cost worth reporting, including at one core.** Every codec is within a few percent of uncompressed on both payloads and both core counts, in both directions — zstd was nominally the *fastest* row twice. Any earlier expectation that compression would visibly cost throughput at low core counts is not supported by these numbers. +- **The size effect is entirely about the data.** On the compressible shape zstd is **14.7x smaller** than uncompressed (57.2 → 3.9 bytes/record) and snappy 3.9x. On genuinely random content both save about 2%, because there is nothing to compress. Parquet's dictionary and byte-array encodings run before any codec, so a repetitive column is already compact and the codec adds little; the gain lives in high-entropy columns *that are not random*, which neither of these shapes represents. +- **Local object storage understates the case for compression.** Uploads here are to a container on the same machine, so the bytes saved buy less time than they would against a remote endpoint. The direction of the trade-off would not reverse. +- Caveat on all of the above: n=1 per point, one machine, no repetition — read these as order-of-magnitude and direction, not as precise figures. + +**Note on defaults, worth knowing before reading the table:** a table created through the Iceberg Go library — which includes tables this output creates itself — comes back carrying `write.parquet.compression-codec: zstd` in its properties, materialised at creation. Since an unset `parquet.compression` defers to the table property, such tables get **zstd**, not the uncompressed default that applies only to a table whose property is absent. The uncompressed rows above required setting the property explicitly. + +To reproduce: as above, with `-iceberg.throughput.codec=zstd|snappy|uncompressed` and `-iceberg.throughput.payload=regular|high-entropy`. + +--- + ## Tuning Recipes The single most important factor for `iceberg` throughput is **records per commit**. Each catalog diff --git a/docs/modules/components/pages/outputs/iceberg.adoc b/docs/modules/components/pages/outputs/iceberg.adoc index 6196fd8df6..4c2d11f84f 100644 --- a/docs/modules/components/pages/outputs/iceberg.adoc +++ b/docs/modules/components/pages/outputs/iceberg.adoc @@ -314,11 +314,11 @@ Compression of the parquet data files this output writes is resolved per table, NOTE: Use a lower-case codec name in the property. This output accepts any casing, but the Iceberg library's own lookup is lower-case only and silently falls back to uncompressed for anything else — so a property of `ZSTD` would give you compressed appends and uncompressed rewrites. -NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: files this output writes are uncompressed, while any copy-on-write rewrites and equality-delete files are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data. +NOTE: The uncompressed default applies only to a table whose property is genuinely absent. A table created through the Iceberg library — which includes tables this output creates itself — comes back carrying `write.parquet.compression-codec: zstd`, set at creation, so an otherwise unconfigured pipeline writing to such a table produces `zstd` throughout rather than anything uncompressed. Either way a table may end up holding a mixture of codecs, which is legal and transparent: parquet records its codec per column chunk, readers handle mixed files, and changing compression never requires rewriting existing data. *Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4_raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this. -*Cost.* Compression trades CPU for bytes. Profiling this output at one vCPU attributes roughly a seventh of its CPU to parquet encoding and finds it CPU bound per record at low core counts, so expect some throughput cost there — though the size of that cost has not yet been measured per codec, so treat it as a reason to measure your own workload rather than as a number. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most. +*Cost.* Less than you might expect. Measuring the write path against local object storage found every codec within a few percent of uncompressed, in both directions, at one core as well as four — so there is no throughput penalty worth planning around. What varies enormously is the size benefit, and that depends on the data: on a repetitive record shape `zstd` was ~15x smaller than uncompressed, while on genuinely random content it saved ~2%, because parquet's dictionary and byte-array encodings have already compacted what they can before any codec runs. See the benchmark results for the numbers and their caveats. == Performance @@ -1238,7 +1238,7 @@ Setting the table property rather than this field is usually the better choice, Only codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4_raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead. -Compression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat. +Measurement found no throughput penalty worth planning around, at one core or four; the size benefit though is entirely data-dependent, ranging from ~15x smaller on a repetitive record shape to ~2% on random content. Note also that the uncompressed fallback applies only when the table property is absent, and tables created through the Iceberg library carry a `zstd` property by default. See <> for the resolution order, the copy-on-write caveat and the measured numbers. *Type*: `string` diff --git a/internal/impl/iceberg/config.go b/internal/impl/iceberg/config.go index 16f3c0a7d6..53c51adc0b 100644 --- a/internal/impl/iceberg/config.go +++ b/internal/impl/iceberg/config.go @@ -122,11 +122,11 @@ const compressionDocs = "\n" + "\n" + "NOTE: Use a lower-case codec name in the property. This output accepts any casing, but the Iceberg library's own lookup is lower-case only and silently falls back to uncompressed for anything else — so a property of `ZSTD` would give you compressed appends and uncompressed rewrites.\n" + "\n" + - "NOTE: A consequence of the above is that a table left entirely unconfigured already contains a mixture: files this output writes are uncompressed, while any copy-on-write rewrites and equality-delete files are `zstd`. This is legal and readable — parquet records its codec per column chunk, so readers handle mixed files transparently, and changing compression never requires rewriting existing data.\n" + + "NOTE: The uncompressed default applies only to a table whose property is genuinely absent. A table created through the Iceberg library — which includes tables this output creates itself — comes back carrying `write.parquet.compression-codec: zstd`, set at creation, so an otherwise unconfigured pipeline writing to such a table produces `zstd` throughout rather than anything uncompressed. Either way a table may end up holding a mixture of codecs, which is legal and transparent: parquet records its codec per column chunk, readers handle mixed files, and changing compression never requires rewriting existing data.\n" + "\n" + "*Codec support.* `snappy`, `gzip` and `zstd` are read by every engine this output targets. Parquet permits others that are not offered here: the original `lz4` codec was ambiguously specified and readers disagree on what it means, `lz4_raw` is its unambiguous replacement but has younger and less universal reader support, and `brotli` and `lzo` are patchily supported. If the table property names one of those it is reported in the log and the files this output writes are uncompressed instead. That governs only those files — the Iceberg library does map those codecs, so if the intent is that nothing writes one, change the table property rather than relying on this.\n" + "\n" + - "*Cost.* Compression trades CPU for bytes. Profiling this output at one vCPU attributes roughly a seventh of its CPU to parquet encoding and finds it CPU bound per record at low core counts, so expect some throughput cost there — though the size of that cost has not yet been measured per codec, so treat it as a reason to measure your own workload rather than as a number. Note also that parquet applies dictionary and run-length encoding before any codec, so repetitive columns are already compact and may gain little; high-entropy payloads gain the most.\n" + "*Cost.* Less than you might expect. Measuring the write path against local object storage found every codec within a few percent of uncompressed, in both directions, at one core as well as four — so there is no throughput penalty worth planning around. What varies enormously is the size benefit, and that depends on the data: on a repetitive record shape `zstd` was ~15x smaller than uncompressed, while on genuinely random content it saved ~2%, because parquet's dictionary and byte-array encodings have already compacted what they can before any codec runs. See the benchmark results for the numbers and their caveats.\n" // rowOperationDocs is the long-form documentation for the row-level operation // feature. It lives in the component description rather than inline in the @@ -526,7 +526,7 @@ array:list Default("delta_length_byte_array"), service.NewStringEnumField(ioFieldParquetCompression, "uncompressed", "snappy", "gzip", "zstd"). - Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4_raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nCompression trades CPU for size, and this output is CPU bound per record at low core counts, so expect some throughput cost on a small deployment — the per-codec cost has not been measured, so measure your own workload rather than assuming. See <> for the full resolution order and the copy-on-write caveat."). + Description("The compression codec for data files this output writes. **Optional on purpose**: when it is not set, the codec is taken from the table's own `write.parquet.compression-codec` property, and when that is absent too, data files are written uncompressed.\n\nSetting the table property rather than this field is usually the better choice, because the property is also honoured by the copy-on-write rewrite path (which writes its files inside the Iceberg library, out of reach of this field) — so the property is what gets every file in the table onto one codec, whereas this field governs only the data files this output writes itself — appends and merge-on-read data files, not copy-on-write rewrites or equality-delete files. Use this field when the property cannot be set, for example on catalogs that reject client-set table properties.\n\nOnly codecs that every engine this output targets can read are offered. If the table property names something else (`lz4`, `lz4_raw`, `brotli`, `lzo`), it is reported in the log and the data files this output writes are uncompressed instead.\n\nMeasurement found no throughput penalty worth planning around, at one core or four; the size benefit though is entirely data-dependent, ranging from ~15x smaller on a repetitive record shape to ~2% on random content. Note also that the uncompressed fallback applies only when the table property is absent, and tables created through the Iceberg library carry a `zstd` property by default. See <> for the resolution order, the copy-on-write caveat and the measured numbers."). ShortDescription("Compression codec for written data files. Defaults to the table's write.parquet.compression-codec property, else uncompressed."). Optional(), ).Description("Parquet writer configuration."). diff --git a/internal/impl/iceberg/integration/throughput_bench_test.go b/internal/impl/iceberg/integration/throughput_bench_test.go new file mode 100644 index 0000000000..d3e41e619a --- /dev/null +++ b/internal/impl/iceberg/integration/throughput_bench_test.go @@ -0,0 +1,302 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/redpanda/blob/master/licenses/rcl.md + +package iceberg + +import ( + "bytes" + "context" + "flag" + "fmt" + "io" + "math/rand" + "strconv" + "strings" + "testing" + "time" + + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/catalog" + iceio "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/table" + "github.com/parquet-go/parquet-go" + "github.com/stretchr/testify/require" + + "github.com/redpanda-data/benthos/v4/public/service" +) + +// Sink write-path throughput against the containerised MinIO + Iceberg REST +// catalog, driving the Router directly. +// +// Why not the pipeline benchmark under bench/: that runs the assembled +// `iceberg` output, which is an enterprise component and so needs a valid +// licence to initialise. This measures the same write path one layer down — +// JSON decode, shredding, parquet encode, upload, catalog commit — by driving +// the Router the way the integration tests already do, which needs no licence. +// +// What it therefore does NOT include, and what the numbers should not be read +// as covering: benthos input, pipeline and batching overhead, and object +// storage that behaves like a real cloud endpoint rather than a local +// container. It is a comparative instrument — before/after a change, or codec +// against codec — not an absolute throughput figure for a deployment. +var ( + throughputRun = flag.Bool("iceberg.throughput", false, + "run the write-path throughput measurement (needs Docker; takes minutes)") + throughputRecords = flag.Int("iceberg.throughput.records", 200000, + "records to write per run") + throughputBatch = flag.Int("iceberg.throughput.batch", 5000, + "records per Route call") + throughputCodec = flag.String("iceberg.throughput.codec", "", + "value for the table's write.parquet.compression-codec property; empty leaves it unset") + throughputLabel = flag.String("iceberg.throughput.label", "run", + "label to print alongside the result, for telling A/B runs apart") + throughputColumns = flag.Int("iceberg.throughput.columns", 0, + "extra string columns beyond the base five; the shredder's per-record cost scales with schema width, so this is the axis its optimisation targets") + throughputPayload = flag.String("iceberg.throughput.payload", "regular", + "record shape: 'regular' (~90B, sequential ids and a shared string prefix) or 'high-entropy' (~1.2kB of random text)") +) + +// TestWriteThroughput writes a fixed number of records and reports the rate and +// the bytes they occupy, so a change to the write path can be measured +// before/after and so the cost of a compression codec can be quantified. +func TestWriteThroughput(t *testing.T) { + if !*throughputRun { + t.Skip("set -iceberg.throughput to run the write-path throughput measurement") + } + + ctx := t.Context() + infra := setupTestInfra(t, ctx) + + const namespace = "bench" + infra.CreateNamespace(t, namespace) + + tableName := fmt.Sprintf("tput_%d", time.Now().UnixNano()) + + // Pre-create the table so the measured window contains no CREATE TABLE, and + // so the compression codec can be set through the property the resolver + // actually reads. Schema mirrors the localhost bench config's record shape. + fields := []iceberg.NestedField{ + {ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64}, + {ID: 2, Name: "user_id", Type: iceberg.PrimitiveTypes.Int64}, + {ID: 3, Name: "event_type", Type: iceberg.PrimitiveTypes.String}, + {ID: 4, Name: "value", Type: iceberg.PrimitiveTypes.Int64}, + {ID: 5, Name: "info", Type: iceberg.PrimitiveTypes.String}, + } + for i := range *throughputColumns { + fields = append(fields, iceberg.NestedField{ + ID: 100 + i, Name: fmt.Sprintf("col_%d", i), Type: iceberg.PrimitiveTypes.String, + }) + } + sc := iceberg.NewSchema(0, fields...) + + client := infra.NewCatalogClient(t, namespace) + var opts []catalog.CreateTableOpt + if *throughputCodec != "" { + opts = append(opts, catalog.WithProperties(iceberg.Properties{ + "write.parquet.compression-codec": *throughputCodec, + })) + } + _, err := client.CreateTable(ctx, tableName, sc, opts...) + require.NoError(t, err) + + // Log what the table actually carries: whether a property was set here or + // materialised by the catalog decides which codec the resolver picks. + if created, err := client.LoadTable(ctx, tableName); err == nil { + t.Logf("TABLEPROPS %v", created.Properties()) + } + + router := infra.NewRouter(t, namespace, tableName) + + // Build every batch up front so record generation is not inside the timed + // window — the point of measurement is the sink, not the generator. + batches := buildThroughputBatches(t, *throughputRecords, *throughputBatch, *throughputPayload, *throughputColumns) + + start := time.Now() + for _, batch := range batches { + require.NoError(t, router.Route(ctx, batch)) + } + elapsed := time.Since(start) + + // Bytes come from the snapshot summary rather than being counted locally, so + // the figure is what the table actually holds. + tbl, err := client.LoadTable(ctx, tableName) + require.NoError(t, err) + var committedRecords int64 + summary := map[string]string{} + if snap := tbl.CurrentSnapshot(); snap != nil && snap.Summary != nil { + summary = snap.Summary.Properties + committedRecords, _ = strconv.ParseInt(summary["total-records"], 10, 64) + } + + // Size is summed from the manifests rather than taken from the snapshot + // summary's total-files-size, which this catalog does not populate with the + // data-file total — it reported 82kB for a table whose string column alone + // reads back as 528kB, so it cannot be used for a bytes-per-record figure. + totalBytes, dataFiles, writtenCodec := sumDataFileBytes(t, ctx, tbl) + + // Assert the codec that was actually written, rather than assuming the + // property took effect. Without this a run that silently ignored the + // property would produce a plausible-looking size comparison. + wantCodec := "UNCOMPRESSED" + if *throughputCodec != "" { + wantCodec = strings.ToUpper(*throughputCodec) + } + require.Equal(t, wantCodec, writtenCodec, + "data files were written with %s, not the requested %s", writtenCodec, wantCodec) + + // Refuse to report a rate for a run that did not actually land the data. A + // throughput number from a partial or failed write looks entirely plausible + // and is worthless, so this is asserted rather than trusted. + require.EqualValues(t, *throughputRecords, committedRecords, + "table holds %d records, expected %d — the measured rate would be meaningless (summary: %v)", + committedRecords, *throughputRecords, summary) + require.Positive(t, totalBytes, "table reports no file bytes (summary: %v)", summary) + + // Read the data back and confirm the columns carry real content. Row counts + // alone would not catch a column silently arriving empty, which would make + // any bytes-per-record figure nonsense. + type contentRow struct { + N int64 `json:"n"` + InfoLen int64 `json:"info_len"` + Distinct int64 `json:"distinct_info"` + } + content := querySQL[contentRow](t, ctx, infra, fmt.Sprintf( + `SELECT count(*) AS n, sum(length(info)) AS info_len, count(DISTINCT info) AS distinct_info FROM iceberg_cat."%s"."%s";`, + namespace, tableName)) + require.Len(t, content, 1) + require.EqualValues(t, *throughputRecords, content[0].N, "read-back row count") + require.Positive(t, content[0].InfoLen, "the info column read back empty") + t.Logf("READBACK rows=%d info_bytes=%d distinct_info=%d", + content[0].N, content[0].InfoLen, content[0].Distinct) + + records := int64(*throughputRecords) + rate := float64(records) / elapsed.Seconds() + perRecord := 0.0 + if records > 0 { + perRecord = float64(totalBytes) / float64(records) + } + + t.Logf("RESULT label=%s payload=%s cols=%d codec=%q written=%s records=%d batch=%d files=%d elapsed=%s rec/s=%.0f bytes=%d bytes/rec=%.1f", + *throughputLabel, *throughputPayload, 5+*throughputColumns, *throughputCodec, writtenCodec, records, *throughputBatch, dataFiles, + elapsed.Round(time.Millisecond), rate, totalBytes, perRecord) +} + +// sumDataFileBytes totals the on-disk size of every data file the table's +// current snapshot references, and returns that with the file count. +func sumDataFileBytes(t *testing.T, ctx context.Context, tbl *table.Table) (bytes, files int64, codec string) { + t.Helper() + snap := tbl.CurrentSnapshot() + require.NotNil(t, snap, "table has no snapshot") + + fs, err := tbl.FS(ctx) + require.NoError(t, err) + + manifests, err := snap.Manifests(fs) + require.NoError(t, err) + + for _, m := range manifests { + for e, err := range m.Entries(fs, true) { + require.NoError(t, err) + df := e.DataFile() + if df.ContentType() != iceberg.EntryContentData { + continue + } + bytes += df.FileSizeBytes() + files++ + + // Every data file should carry the same codec; read one and check + // the rest agree, so a partially-applied setting cannot hide. + fileCodec := parquetCodecOf(t, fs, df.FilePath()) + if codec == "" { + codec = fileCodec + } + require.Equal(t, codec, fileCodec, + "data files disagree on codec: %s vs %s", codec, fileCodec) + } + } + return bytes, files, codec +} + +// parquetCodecOf reports the compression codec recorded in a parquet file's +// footer, read from object storage. +func parquetCodecOf(t *testing.T, fs iceio.IO, path string) string { + t.Helper() + f, err := fs.Open(path) + require.NoError(t, err) + defer f.Close() + + data, err := io.ReadAll(f) + require.NoError(t, err) + + pf, err := parquet.OpenFile(bytesReaderAt(data), int64(len(data))) + require.NoError(t, err) + + for _, rg := range pf.Metadata().RowGroups { + for _, col := range rg.Columns { + return col.MetaData.Codec.String() + } + } + t.Fatalf("parquet file %s has no column chunks", path) + return "" +} + +// bytesReaderAt adapts a byte slice to the io.ReaderAt parquet needs. +func bytesReaderAt(b []byte) *bytes.Reader { return bytes.NewReader(b) } + +// buildThroughputBatches produces batches of JSON records in one of two shapes. +// +// The shape matters a great deal for anything size-related, which is why it is +// selectable rather than fixed. "regular" mirrors the localhost bench config's +// generator: sequential ids and an `info` string sharing a 21-character prefix. +// Parquet's byte-array encoding compresses that prefix away before any codec +// runs, so the same 20k records occupy ~4 bytes each with no compression at all +// — measuring a codec against it would show almost no win and imply, wrongly, +// that compression does not pay. "high-entropy" fills `info` with random text +// instead, which is the regime where a codec earns its cost. +func buildThroughputBatches(t *testing.T, total, perBatch int, payload string, extraColumns int) []service.MessageBatch { + t.Helper() + require.Contains(t, []string{"regular", "high-entropy"}, payload, + "unknown payload shape %q", payload) + + eventTypes := []string{"click", "view", "purchase", "scroll", "hover"} + const alnum = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + + // Deterministic, but freshly generated per record rather than sliced from a + // shared pool. Slicing a 64kB pool 100k times produces values that overlap + // heavily, and parquet's encoders exploit that redundancy across records — + // the result looked like 4:1 compression on supposedly random data, which + // would have made any codec comparison meaningless. + rng := rand.New(rand.NewSource(42)) //nolint:gosec // benchmark entropy, not crypto + const highEntropyLen = 1100 + + var batches []service.MessageBatch + for start := 0; start < total; start += perBatch { + n := min(perBatch, total-start) + batch := make(service.MessageBatch, n) + for i := range n { + id := start + i + info := fmt.Sprintf("event info for record %d", id) + if payload == "high-entropy" { + buf := make([]byte, highEntropyLen) + for j := range buf { + buf[j] = alnum[rng.Intn(len(alnum))] + } + info = string(buf) + } + var extra strings.Builder + for c := range extraColumns { + fmt.Fprintf(&extra, `,"col_%d":"v%d_%d"`, c, c, id%97) + } + batch[i] = service.NewMessage(fmt.Appendf(nil, + `{"id":%d,"user_id":%d,"event_type":%q,"value":%d,"info":%q%s}`, + id, id%10000+1, eventTypes[id%5], id%1000, info, extra.String())) + } + batches = append(batches, batch) + } + return batches +} From f82e5f166af5e62f97c6b91b402a0051bdc62ced Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Thu, 27 Aug 2026 15:35:18 +0100 Subject: [PATCH 11/12] iceberg: fix the throughput harness's codec expectation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run without -iceberg.throughput.codec, the harness always aborted. It creates its table through the Iceberg library, which materialises write.parquet.compression-codec: zstd at creation, and an unset field defers to the table property — so the write resolved to zstd while the assertion expected UNCOMPRESSED whenever the flag was empty. I had actually seen this failure, diagnosed the library behaviour behind it, and written that up without fixing the assertion that exposed it. The expectation is now derived from the resolution rule rather than the flag: an explicit setting wins, otherwise the table's property applies, otherwise uncompressed, with declined and unrecognised codec names falling back to uncompressed. Deliberately restated in the test rather than calling into resolveParquetCompression, which is unexported in another package and would mean asserting the implementation against itself. Kept the flag's unset case reachable rather than defaulting it to an explicit codec, because "no field set, whatever the table says" is a real user configuration and worth exercising. The flag's help text now says what unset actually resolves to, since "leaves it unset" read as "expect uncompressed". No effect on the recorded results: every row in the compression table came from an explicit -codec run, and the shredder A/B held compression at an explicit uncompressed, so all published numbers were measured with the assertion agreeing. --- .../integration/throughput_bench_test.go | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/internal/impl/iceberg/integration/throughput_bench_test.go b/internal/impl/iceberg/integration/throughput_bench_test.go index d3e41e619a..8e9277b74a 100644 --- a/internal/impl/iceberg/integration/throughput_bench_test.go +++ b/internal/impl/iceberg/integration/throughput_bench_test.go @@ -52,7 +52,7 @@ var ( throughputBatch = flag.Int("iceberg.throughput.batch", 5000, "records per Route call") throughputCodec = flag.String("iceberg.throughput.codec", "", - "value for the table's write.parquet.compression-codec property; empty leaves it unset") + "value for the table's write.parquet.compression-codec property; empty leaves it unset, in which case the codec is whatever the table resolves to — note the Iceberg library materialises zstd at table creation, so unset does NOT mean uncompressed here") throughputLabel = flag.String("iceberg.throughput.label", "run", "label to print alongside the result, for telling A/B runs apart") throughputColumns = flag.Int("iceberg.throughput.columns", 0, @@ -104,11 +104,13 @@ func TestWriteThroughput(t *testing.T) { _, err := client.CreateTable(ctx, tableName, sc, opts...) require.NoError(t, err) - // Log what the table actually carries: whether a property was set here or - // materialised by the catalog decides which codec the resolver picks. - if created, err := client.LoadTable(ctx, tableName); err == nil { - t.Logf("TABLEPROPS %v", created.Properties()) - } + // Capture what the table actually carries: whether a property was set here + // or materialised by the catalog decides which codec the writer resolves to, + // and the assertion below has to expect the same thing. + created, err := client.LoadTable(ctx, tableName) + require.NoError(t, err) + tableProps := created.Properties() + t.Logf("TABLEPROPS %v", tableProps) router := infra.NewRouter(t, namespace, tableName) @@ -142,12 +144,10 @@ func TestWriteThroughput(t *testing.T) { // Assert the codec that was actually written, rather than assuming the // property took effect. Without this a run that silently ignored the // property would produce a plausible-looking size comparison. - wantCodec := "UNCOMPRESSED" - if *throughputCodec != "" { - wantCodec = strings.ToUpper(*throughputCodec) - } + wantCodec := expectedCodec(*throughputCodec, tableProps) require.Equal(t, wantCodec, writtenCodec, - "data files were written with %s, not the requested %s", writtenCodec, wantCodec) + "data files were written with %s; expected %s from codec flag %q and table property %q", + writtenCodec, wantCodec, *throughputCodec, tableProps[parquetCompressionProperty]) // Refuse to report a rate for a run that did not actually land the data. A // throughput number from a partial or failed write looks entirely plausible @@ -186,6 +186,33 @@ func TestWriteThroughput(t *testing.T) { elapsed.Round(time.Millisecond), rate, totalBytes, perRecord) } +// parquetCompressionProperty is Iceberg's table property for the codec data +// files are written with. +const parquetCompressionProperty = "write.parquet.compression-codec" + +// expectedCodec restates the output's own resolution rule, so the assertion +// checks behaviour against the documented contract rather than against a fixed +// value: an explicit setting wins, otherwise the table's property applies, +// otherwise uncompressed. Codecs the output declines to write, and values it +// does not recognise, both fall back to uncompressed. +// +// Deliberately a restatement rather than a call into the resolver — that lives +// in an unexported function in another package, and asserting an implementation +// against itself would prove nothing. +func expectedCodec(configured string, props iceberg.Properties) string { + name := strings.ToLower(strings.TrimSpace(configured)) + if name == "" { + name = strings.ToLower(strings.TrimSpace(props[parquetCompressionProperty])) + } + switch name { + case "snappy", "gzip", "zstd": + return strings.ToUpper(name) + default: + // uncompressed, none, absent, declined (lz4/lz4_raw/brotli/lzo) or junk. + return "UNCOMPRESSED" + } +} + // sumDataFileBytes totals the on-disk size of every data file the table's // current snapshot references, and returns that with the file count. func sumDataFileBytes(t *testing.T, ctx context.Context, tbl *table.Table) (bytes, files int64, codec string) { From a26e7fe15c180fc55724de85caa7b95887069260 Mon Sep 17 00:00:00 2001 From: Ashley Jeffs Date: Thu, 27 Aug 2026 15:36:43 +0100 Subject: [PATCH 12/12] iceberg: correct the warnedCompression field comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment still said the map is keyed by warning text. It has been keyed per table plus warning text since the commit that made compression warnings name their table — so the field comment described precisely the bug that commit fixed, while the function's own doc comment explained the fix correctly. --- internal/impl/iceberg/router.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/impl/iceberg/router.go b/internal/impl/iceberg/router.go index 447b19a87e..3906758953 100644 --- a/internal/impl/iceberg/router.go +++ b/internal/impl/iceberg/router.go @@ -147,8 +147,10 @@ type Router struct { entries sync.Map // tableKey -> *tableEntry - // warnedCompression de-duplicates compression warnings, keyed by warning - // text, so rebuilding a writer does not re-log one. See warnCompressionOnce. + // warnedCompression de-duplicates compression warnings, keyed by table and + // warning text together: rebuilding a writer does not re-log a warning for + // that table, but a second table hitting the same problem is still reported. + // See warnCompressionOnce. warnedCompression sync.Map // parquetCompression is the configured `parquet.compression` value, or ""