iceberg: cut shredder allocations, and add write/commit performance harnesses

[Jeffail]

Some iceberg sink performance work. One production change plus the measurement tooling that motivated it — happy to split them if you'd rather review separately.

The production change: shredder allocations (2e80f3e)

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 for case-insensitive matching, where several input keys can fold onto one schema field and that ambiguity needs reporting rather than silently resolving.

When matching is case-sensitive though, 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 can't happen because Go map keys are themselves case-sensitive. That's the default (case_sensitive_columns: true), so from what I can tell it's the path almost all traffic takes.

So this splits the two apart — shredStructExact for case-sensitive matching with direct lookups, and 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're all accounted for (the steady state once a schema settles), building a lookup set only on the rare path where something genuinely is unknown.

BenchmarkShredWide at GOMAXPROCS=1, benchstat, n=8:

                sec/op                      B/op                        allocs/op
before          4.369µ                      4.312Ki                     71
after           1.472µ  -66.3% (p=0.000)    1.609Ki  -62.7% (p=0.000)    41  -42.3% (p=0.000)

Worth being clear that's the shredder in isolation. Earlier profiling put shredding at roughly 27% of the sink's CPU, so my read is the end-to-end saving should be appreciable but a good deal smaller than 66% — I haven't measured that yet, so please treat the sink-level number as unquantified rather than implied.

Since it's purely an optimisation it shouldn't change observable behaviour at all, so TestShredStructPathsAgree runs the same schema and record through both implementations and requires identical emitted values (in order), identical new-field notifications (compared as sets, since map iteration order is random) and identical errors. Twelve cases: full matches, the exact-match steady state, one and several unknown keys, an unknown key nested inside a known struct, missing optionals, explicit nulls, both required-field error paths, and the empty-record / empty-schema edges.

The tooling (17f8f54, 42e4bc6)

No production code, all flag-gated or skipped by default:

1. A throughput bench for the Databricks e2e harness plus local profiling configs and a wide-schema shredder micro-benchmark (the one the numbers above come from).
2. A commit-regime harness — wraps the existing in-memory test catalog with a configurable per-commit delay and a commit counter, then drives N concurrent submitters at it and reports 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 encode and upload cost don't confound it.

A fixed injected delay seems like a fair 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 looks roughly constant with respect to batch size — and unlike a hosted service you can sweep it across regimes instead of being pinned to one.

[claude]

Test coverage: the nested comparison in this test is vacuous.

The shredder is constructed with caseSensitive: true, and only the top-level call is dispatched explicitly. Nested structs are reached via shredValue → shredStruct, and shredStruct dispatches on rs.caseSensitive — so it routes to shredStructExact for both the "exact" and the "folded" run.

Result: below the top level both sinks are produced by the same implementation, so the "unknown key nested inside a known struct" and "all fields present" cases compare shredStructExact against itself. Any divergence in nested struct handling (nested unknown-field notification, nested required-field errors, nested path cloning) cannot be detected, even though the test doc-comment and the commit message both claim nested coverage — and this test is the stated safety net for a behaviour-preserving optimisation.

Suggested fix: drive the comparison through the public entry point with two shredders instead of calling the internal helpers directly — NewRecordShredder(schema, true).Shred(record, sink) vs. NewRecordShredder(schema, false).Shred(record, sink) — so the whole recursion (top level and nested) goes exact-all-the-way on one side and folded-all-the-way on the other. Keep the direct shredStructExact/shredStructFolded calls only if you additionally want the top-level-only assertion.

[Jeffail]

Good catch, and you are right that it was vacuous below the root — shredStruct dispatches on rs.caseSensitive on every recursion, so with one case-sensitive shredder both runs went through shredStructExact for nested structs and the test compared the fast path against itself. Fixed in 1bf1770 by driving both runs through the public Shred entry point with two shredders, as you suggested, so one side is exact all the way down and the other folded all the way down.

I dropped the direct helper calls entirely rather than keeping them alongside — the public-entry comparison subsumes the top-level assertion, so keeping both seemed like it would only add a second thing to keep in sync.

Two things I added on top:

1. Three doubly-nested cases, so the nested error and notification paths are actually reachable: an unknown key two levels down, and a required leaf both missing and explicitly null two levels down.
2. A check that the test now has teeth — I injected a mutation that drops nested unknown-field notifications from the fast path only, and both nested cases fail, where by your argument they could not have before.

One caveat now called out in the doc-comment: the comparison is only legitimate for a case-unambiguous corpus, so every field name and record key in it is lower-case. Input that differs in case is exactly where the two modes are meant to diverge, so that belongs in the case-sensitivity tests rather than here.

[claude]

Benchmark results not recorded (CONTRIBUTING.md §1.3.4).

This is a hot-path serialization change with measured throughput numbers quoted in the commit message (sec/op 4.369µ → 1.472µ, -62.7% B/op), and the PR also adds files under internal/impl/iceberg/bench/. CONTRIBUTING.md §1.3.4 requires following the reporting requirements in docs/benchmarking.md and recording results under docs/benchmark-results/, and docs/benchmarking.md "Keeping Results Up to Date" is explicit on both counts:

2. When modifying a connector's performance path — Re-run the benchmark and append a new dated section to the results file. This includes changes to batching, buffering, connection handling, serialization, or any code that sits in the hot path.
3. During code review — […] It will flag PRs that add or modify bench/ directories without updating results files, and PRs that include throughput numbers in the description without recording them in docs/benchmark-results/.

docs/benchmark-results/iceberg.md already exists but is untouched by this PR. Please append a dated section to it with the BenchmarkShredWide before/after numbers (plus environment and PR link). The commit message notes the end-to-end effect "has not been measured yet" — that's fine to state in the section, but the micro-benchmark delta and the new bench harnesses still need recording there.

Separately, profile_config.yaml / profile_config_schema.yaml are not mentioned in internal/impl/iceberg/bench/README.md nor wired into the directory's Taskfile.yaml, which docs/benchmarking.md §4 and §6 ask for.

[Jeffail]

Fair, and I had missed that docs/benchmarking.md is explicit about both halves of this. Addressed in e9c8bd2.

Appended two dated sections to docs/benchmark-results/iceberg.md: the BenchmarkShredWide before/after (benchstat over n=8, with environment, PR link and what changed) and the commit-regime sweep, each with its reproduction command.

I have tried to make both hard to over-read, since that felt like the real risk in recording a -66%:

• the shredder section states plainly that the number is the micro-benchmark in isolation, that earlier profiling put shredding at ~27% of sink CPU so the end-to-end effect should be much smaller, and that no sink-level throughput figure in the file has been re-measured for this change;
• the commit-regime section notes it writes no parquet and touches no object storage, so its rec/sec are ratios for comparing coalescing behaviour and are not comparable with the localhost or live-catalog sections.

On the second half: profile_config.yaml and profile_config_schema.yaml are now documented in the bench README and wired into the Taskfile as bench:profile and bench:profile:schema, plus a bench:shredder task for the micro-benchmark (needs no infrastructure). I ran bench:shredder to check the wiring rather than just that the YAML parses.

Still outstanding and not something I wanted to guess at: the CHANGELOG entry. 4.105.0 is released and there is no Unreleased heading, so I would be inventing a version number — happy to add it wherever you would like it.

[claude]

require called from a non-test goroutine.

This require.NoError (and the require.NoError inside recordCountDataFile, also reached from these goroutines) runs on a goroutine that is not the one running the test. require failures call t.FailNow(), which testify documents as only valid from the test goroutine — it invokes runtime.Goexit(), so on a Commit failure the goroutine unwinds mid-way and the test then proceeds to evaluate the commits assertions against a half-completed run, producing a confusing failure rather than the real cause.

The project's test patterns call this out directly: "require calls FailNow() which panics when called from a non-test goroutine."

Suggested fix: have each goroutine capture its error (e.g. into a per-index slice or an errors.Join-able collector, or build the data files on the test goroutine before spawning) and assert with require after wg.Wait() on the test goroutine. assert.NoError inside the goroutine would also be safe if you prefer to keep the check in place.

[Jeffail]

You are right, and thanks — this one would have been genuinely misleading in failure. require calls FailNow, which is only valid on the test goroutine, so on a commit failure the submitter unwound mid-loop via runtime.Goexit and the test then evaluated its commit-count assertions against a half-finished run, reporting a count mismatch rather than the actual error.

Fixed in 1bf1770. recordCountDataFile now returns an error instead of asserting, submitters record their first error, and the test goroutine asserts on errors.Join(errs...) after wg.Wait() — before deriving any rate, since a rate from a partial run would be meaningless anyway. The coalescing test now builds its data files up front on the test goroutine so its submitters only have to commit. Both pass under -race.

[claude]

The table-property lookup is case-sensitive, but the property value is not this output's to normalise. parquetCompressionCodecs is keyed on lower-case names, so a table carrying write.parquet.compression-codec: ZSTD (or Snappy, GZIP) misses the map and falls through to the declined-codec branch below — the files are written uncompressed and the operator gets a warning that says the codec is one "readers disagree on it or engine support is patchy", which is untrue for a plain casing difference. Meanwhile the copy-on-write rewrite path inside iceberg-go keeps honouring the property, so the same table ends up with a codec mismatch that the resolution order documented above is specifically meant to prevent.

The resolver already goes out of its way to accept both none and uncompressed spellings from the property (lines 61-67), so casing looks like the same class of property-value variance rather than an intentional exclusion. Suggest folding fromTable to lower case before the lookup (the configured value on line 74 does not need it — the enum validates it), and adding a mixed-case property case to TestResolveParquetCompression, which currently covers none, "", lz4, brotli and nonsense but no casing variant.

Relevant to CONTRIBUTING.md §3.1.4 ("implementation is complete and correct") and §3.2.2 (hard-to-diagnose error handling — the warning misdirects).

[Jeffail]

Good catch on the casing — folded in ad6056f4, and it turned out to be worse than described, in a way I would not have found without this.

The property fold is in. But an independent pass then established that the configured value needs folding too, which I had explicitly argued it did not: the enum linter lower-cases before comparing against the option set, so compression: ZSTD passes validation and arrives at the resolver verbatim, misses the map, and silently writes uncompressed while logging a warning that reads like an internal-invariant violation. So the comment asserting "an invalid configured value cannot reach here" was simply wrong, and both sides now fold. Tests cover casing variants on each side, plus whitespace-only property values, plus the warning text.

One correction to the reasoning though, in case it matters elsewhere: I do not think the mismatch-with-copy-on-write consequence held. iceberg-go's own codec switch is also lower-case only (table/internal/parquet_files.go) — ZSTD falls to its default:, which warns and leaves the codec at its zero value, and that zero value is uncompressed. So before this change both paths wrote uncompressed and agreed; the fold is what introduces a divergence, since our appends now honour ZSTD while the library's rewrites still do not. Both files remain readable, so it is a documentation problem rather than a correctness one, and the docs now say to use a lower-case codec name and explain why. The real fix belongs upstream in iceberg-go, which I would like to raise separately.

Also split the warning in two while I was here, since you were right that it misdirected: one message for a codec we decline, another for a value we simply do not recognise, so a typo no longer gets told that readers disagree about it.

[claude]

This is the only production wiring for the new feature and it has no test. TestResolveParquetCompression exercises the pure resolver and TestParquetCompressionReachesWrittenFile builds a sink by hand — neither goes through Router.createWriter, so nothing covers:

1. The per-table isolation this comment exists to protect. The slices.Concat-not-append reasoning is correct, but a future append(r.writerOpts, ...) regression would be silent: two tables whose properties resolve to different codecs would share a backing array and one would overwrite the other's option. A router-level test with two tables carrying different write.parquet.compression-codec properties, asserting each writer's resolved codec, would pin it.
2. The config → router path. rtr.parquetCompression is assigned post-construction in output_iceberg.go after a conf.Contains check; nothing asserts that an unset parquet.compression stays "" (so the table property is reachable) while an explicit uncompressed does not. That "unset ≠ uncompressed" distinction is the central design claim of the field and is only asserted at the resolver level, where "" is passed in directly rather than parsed.

Per the project test patterns, changed production code should carry tests; a spec.ParseYAML + MockResources case for (1) unset and (2) explicit uncompressed plus a two-table createWriter case would close both gaps.

Related: CONTRIBUTING.md §1.3.2 — tests should prove the connector works across supported configurations.

[Jeffail]

Agreed on both gaps — closed in ad6056f4.

For the isolation property I extracted the option build into Router.writerOptsFor(props), so it can be driven without standing up a catalog, and TestWriterOptsForIsolatesTables resolves two tables to different codecs and asserts each option independently. The base slice is deliberately constructed with spare capacity (cap 8, len 1), because that is the only condition under which the append version of the bug bites — with a full slice append allocates and the aliasing is invisible. So the test would actually fail on the regression you describe, rather than merely looking like it covers it.

For the config path, TestParquetCompressionConfigParsing goes through ParseYAML for four cases — no parquet block, a parquet block without compression, an explicit uncompressed, and an explicit codec — asserting both what reaches the router and the codec that then resolves against a table property. That pins the "unset ≠ uncompressed" claim where it is actually made rather than at the resolver, which was your point.

I did not add a two-table createWriter test: it needs a live catalog client, so it would be an integration test rather than the unit test the gap calls for. writerOptsFor is the whole of the per-table logic and createWriter's only remaining involvement is passing the table's properties to it, so I think the seam is in a reasonable place — happy to be pushed on that if you disagree.

[claude]

This section is missing the date and PR link that docs/benchmarking.md requires of results sections:

Append new runs as dated sections so we can track performance over time. […] Include the date, PR link, and what changed since the last run.

— docs/benchmarking.md#L288-L328

The Shredder Allocations section added directly above gets this right (## Shredder Allocations — 2026-08-20 plus a Changed since last run line carrying the PR link, lines 218-232); this one carries an Environment line and a caveat but no date and no PR reference, so a later reader cannot tell when the sweep was run or against which revision of the committer — which matters here because the numbers are explicitly about batcher coalescing behaviour that may change. Suggest matching the sibling section's heading and metadata form.

CONTRIBUTING.md §1.3.4 points at docs/benchmarking.md for the reporting requirements.

[Jeffail]

Fixed in ad6056f4 — the section now carries the date and a Changed since last run line with the PR link, matching the sibling section's form.

You picked the right section to worry about: I noted in it that the numbers describe batcher coalescing behaviour specifically, so they should be re-run if the commit batching path changes. Since these are the first run of that harness there is no prior run to diff against, which the line says rather than implying otherwise.

[claude]

This comment is the sole justification for keeping the case-insensitive body inline rather than extracting it, but it cites a benchmark that does not exist. There is no BenchmarkShredWideFolded anywhere in the tree — the benchmark added in this PR for that path is BenchmarkShredCaseInsensitive (shredder_bench_test.go#L79), whose own doc comment records the same ~5% figure. As written, a future reader cannot re-run the measurement that this design decision rests on.

Suggested fix: name BenchmarkShredCaseInsensitive here instead. (BenchmarkShredWide also exists but lives in the bench package and is case-sensitive only, so it is not the one that measured this.)

Ref: .claude/agents/godev.md — Documentation / Logging Over Comments — comments should document why the logic exists, accurately.

[Jeffail]

Quite right, and thanks — that name was a throwaway probe I used to take the measurement, which I then deleted without updating the comment it was justifying. Circular and useless to a future reader. Fixed in e55a882f to name BenchmarkShredCaseInsensitive, which is the benchmark that now guards that path and records the same figure.

[claude]

This PR changes the sink's hot path (the shredder) and adds a new knob that spends CPU per record (parquet.compression), but no localhost or real-endpoint throughput number is re-measured — this section says so explicitly ("It has not been measured end to end — no throughput figure above or elsewhere in this file has been re-run for this change"), and there is no section at all for the compression codec.

docs/benchmarking.md item 2 asks for a re-run, not only an appended section: "When modifying a connector's performance path — Re-run the benchmark and append a new dated section to the results file. This includes changes to batching, buffering, connection handling, serialization, or any code that sits in the hot path." CONTRIBUTING §1.3.4 requires both phases (localhost and real-endpoint) and §1.3.5 requires runs at various throughput levels.

The gap matters most for compression: the field docs assert "this output is per-record CPU bound at low core counts, so enabling it can cost throughput on a small deployment" — that is a performance claim shipped to users with no measurement behind it, on a connector where the bench harness to measure it (task bench:profile, task bench) is right here in this PR.

Suggested fix: run the existing localhost bench suite before/after the shredder change, and once more with parquet.compression: zstd, and append the dated sections. If a re-run is genuinely out of scope for this PR, say so in the PR description and link the follow-up.

[Jeffail]

This is the fairest hit in the review, and I can only half-fix it.

Taking the second half first, because you are right and it was the worse problem: the field docs asserted a throughput cost with nothing behind it. Reworded in e55a882f 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 is unmeasured, so the docs now say that and tell the reader to measure their own workload rather than implying a known result.

On the re-run itself: I tried, and I cannot produce it here. The localhost suite drives the iceberg output, which is an enterprise component, and the license on this machine expired on 2026-08-08 — the pipeline refuses to init the output and exits, so there is no before/after pair to be had. (Worth saying I only found that because I had the runner detect failures; the first run I did reported a perfectly plausible 36,846 msg/s that was in fact an immediate license-error exit. Any throughput number I had reported without that check would have been fiction.)

So rather than leave the gap unexplained, e55a882f records it in docs/benchmark-results/iceberg.md alongside the shredder section: what is missing, why, and exactly what to run once a current license is available — task bench before/after plus one run per codec, at one and four cores. Happy to run it and append the sections the moment I have a working license, and equally happy for that to gate the PR if you would rather the numbers land before it merges.

One thing I would push back on gently: for the shredder change specifically, the sink-level effect is bounded above by profiling that already exists (~27% of CPU in shredding), and the isolated measurement is a benchstat A/B over n=8 with p=0.000. I do not think the end-to-end run would change the decision to make the change — it would size the win, which matters for the docs, not for whether the code is right. The compression knob is the one where measurement genuinely could change the recommended default, and that default is deliberately "uncompressed" precisely so nobody inherits an unmeasured cost.

[Jeffail]

Update: got the numbers, from a different angle. e4547874.

The pipeline suite could not produce them, but it turns out the write path does not need it. license.CheckRunningEnterprise is called by the output constructor, not by the Router — so driving the Router against the containerised MinIO + REST catalog, exactly as the integration tests already do, measures JSON decode, shredding, parquet encode, upload and commit with no licence involved. TestWriteThroughput does that, and both sections are now in docs/benchmark-results/iceberg.md.

Two results contradict what this branch previously asserted, so the docs are corrected rather than the results buried:

1. 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 — zstd was nominally the fastest row twice. The "costs throughput at low core counts" line you pushed on was reasoning, not measurement, and the measurement does not support it. What does vary hugely is size, and purely with the data: zstd was ~15x smaller than uncompressed on a repetitive record shape and ~2% smaller on random content.
2. The shredder change shows no measurable end-to-end gain — at 5 columns or 50, at one core or four, against a 66% reduction in isolation. I repeated the wide-schema runs twice per side because the difference sits inside run-to-run variance. My reading is that this harness is not shredder-bound, and I have written it up that way rather than explained it away: the isolated win is measured, its end-to-end value on these shapes is not demonstrated.

I also found something worth knowing independent of this PR: a table created through the Iceberg library — including tables this output creates itself — comes back carrying write.parquet.compression-codec: zstd, materialised at creation. Since an unset field defers to the property, those tables get zstd, so the "uncompressed default" is narrower than the docs claimed. Corrected.

Three things the harness asserts, because each one caught a plausible-looking lie: it fails unless the table holds exactly the records written (a run once reported 36,846 msg/s while exiting immediately on the licence error); it reads the data back to check columns are populated; and it reads the codec out of a written file footer and fails if it is not the one requested (which is how I found the "uncompressed" baseline was writing zstd). A bytes-per-record figure I initially took from the snapshot summary is also gone — this catalog reports 82kB for a table whose string column alone reads back as 528kB, so it is summed from the manifests now.

Caveats stated in the sections: n=1 per point, one machine, local object storage (which understates the case for compression rather than overstating it), and GOMAXPROCS=1 here constrains the writer while MinIO and the catalog have their own cores — so it is not a 1-vCPU deployment. The full-pipeline figures higher up the file are still not re-run; that suite does need the licence.

[claude]

The compression warning never identifies the table it is about, and the de-duplication then hides every table after the first.

resolveParquetCompression builds warnings from the property key and its value only (e.g. Table property write.parquet.compression-codec is "lz4", which this output does not write... — parquet_compression.go#L1433-L1441), and warnCompressionOnce keys the sync.Map on that text. But a Router is inherently multi-table — namespace/table are interpolated per message and writers are cached per tableKey (router.go#L145-L160) — so with two tables carrying the same bad property value the operator gets exactly one warning that names neither table, and there is no way to work out which table's data files silently went uncompressed.

Suggested fix: pass the table identity (the tableKey, available at the createWriter call site) into the warning text, and key warnedCompression on (tableKey, warning) rather than the message alone. That keeps the unbounded-retry suppression this function exists for while still warning once per affected table.

Per CONTRIBUTING.md §1.2.2 — "Provides relevant logging to support troubleshooting."

[Jeffail]

Agreed, and the multi-table angle is the bit I had not thought about — I was treating the de-duplication as purely a retry-suppression problem and keyed it accordingly. Fixed in 0df23cd7 exactly as you suggest: the warning names the table, and warnedCompression is keyed on (tableKey, warning) so per-table reporting survives while the unbounded-retry suppression it exists for still works.

Added a test for it too, since the behaviour has three parts that could each regress independently: three writer builds for one table log once, a second table carrying the identical property value is still reported rather than swallowed, and a supported codec stays silent.

[claude]

lz4raw is parquet-go's spelling, not the one an Iceberg table property carries, so this entry will not match in practice.

The keys here are compared against write.parquet.compression-codec (via normaliseCodecName(props[table.ParquetCompressionKey])), whose values follow the Parquet codec names. This repo records that spelling as lz4_raw — see the commented-out enum in internal/impl/parquet/processor.go#L80-L82 (/*, "lzo", "brotli", "lz4_raw" */); lz4raw is only parquet-go's own option name, used by processor_encode.go#L280-L283.

So a table property of lz4_raw falls past declinedCompressionCodecs into the final branch and the operator is told the value "is not a compression codec this output recognises" instead of the accurate "this output does not write it" message — which defeats the stated purpose of keeping this map distinct ("so the operator is told which of the two happened"). Behaviour is unaffected (uncompressed either way), only the diagnostic.

Suggested fix: add lz4_raw alongside lz4raw here, and use lz4_raw in the codec list in the field description and the Data file compression docs section so the names match what a user would actually put in the property.

[Jeffail]

Good spot, and you are right about which spelling actually turns up — I had taken lz4raw from parquet-go's option name without checking what an Iceberg property carries, which is the parquet codec name LZ4_RAW. Confirmed against this repo's own parquet processor, which records exactly that spelling in its codec list while lz4raw appears only in the parquet-go encode switch.

Fixed in 0df23cd7. I kept both spellings in the declined set rather than swapping one for the other: iceberg-go's own lookup spells it lz4raw, so depending on which writer set the property either form can appear, and the point of that map is to get the diagnostic right in both cases. The docs now say lz4_raw, since that is what a user would type. Tests cover all three forms (lz4_raw, LZ4_RAW, lz4raw) reaching the declined branch rather than the unrecognised one.

As you say, behaviour was unaffected either way — but "not a codec this output recognises" for a codec we deliberately decline was precisely the misdirection that map was introduced to avoid, so thanks for catching that it was not doing its job.

[claude]

This field comment is stale and now contradicts the code it documents. It says the map is "keyed by warning text", but warnCompressionOnce keys on key.namespace + "\x00" + key.table + "\x00" + warning — per table and warning text. That per-table keying is exactly what the last commit ("name the table in compression warnings") introduced, and its own doc comment on warnCompressionOnce spells out why keying on the message alone would swallow every table after the first. So the field comment describes the bug that was just fixed.

Suggested fix: reword the comment to say the map is keyed per table plus warning text, so a rebuilt writer does not re-log but a second affected table is still reported.

This is a documentation-accuracy issue, not a behaviour bug — the implementation and TestCompressionWarningIsPerTable are correct. Flagged under CONTRIBUTING.md §3.1.2 (self-documenting code) / §3.2.2.

[Jeffail]

Correct, and a nice illustration of the failure mode: I changed the keying and updated warnCompressionOnce's doc comment to explain why, but left the struct field comment describing the behaviour I had just removed — so the field documented the bug and the function documented the fix, three lines apart. Reworded in a26e7fe1 to say keyed per table plus warning text, and why: a rebuilt writer does not re-log, but a second affected table is still reported.

[claude]

NOTE — required benchmark re-run is missing, and this PR is the case the rule exists for.

docs/benchmarking.md item 2: "When modifying a connector's performance path — Re-run the benchmark and append a new dated section to the results file. This includes changes to batching, buffering, connection handling, serialization, or any code that sits in the hot path." This PR modifies the shredder hot path (shredStructExact) and adds a serialization knob (parquet.compression), and this bullet records that no end-to-end figure was re-run for either. CONTRIBUTING.md §1.3.4 requires both the localhost and the real-endpoint phase, and §1.3.5 requires them across throughput levels.

Credit where due: the gap is disclosed rather than papered over, the cause is stated, and the exact follow-up command is recorded — which is far better than a silent omission, and is why this is a note rather than a blocker.

Two things worth resolving before merge, though:

1. The blocker given is an expired enterprise licence, which is an environment problem rather than a property of the change — a maintainer with a current licence can produce the before/after pair. Worth asking one to run it rather than deferring, since the shredder change is the headline of the PR and -66% in isolation is currently the only number backing it.
2. The parquet.compression cost is entirely unmeasured, yet the field ships user-facing guidance that "this output is CPU bound per record at low core counts, so expect some throughput cost" (config.go). One run per codec at one and four cores — already named in this bullet — would turn that into an actual figure.

[Jeffail]

This one is now done, in e4547874 — I think it landed after you wrote this, so it may not have been visible.

Both of your specific asks are covered:

1. The before/after pair exists, without needing anyone's licence. license.CheckRunningEnterprise is called by the output constructor, not by the Router, so driving the Router against the containerised MinIO + REST catalog — as the integration tests already do — measures decode, shredding, parquet encode, upload and commit licence-free. That is TestWriteThroughput, and the section is in this file.
2. The compression cost is measured, one run per codec at one and four cores, on two deliberately different record shapes.

Two of the results contradict what the PR previously claimed, so the docs are corrected rather than the numbers buried:

• Compression has no throughput cost worth planning around — every codec within a few percent of uncompressed, both directions, at one core as well as four. The config.go guidance you quote has been rewritten accordingly; it was reasoning rather than measurement, and the measurement does not support it. The size effect is what actually varies: ~15x smaller with zstd on a repetitive record shape, ~2% on random content.
• The shredder change shows no measurable end-to-end gain — at 5 columns or 50, one core or four, against -66% in isolation. I repeated the wide-schema runs twice per side because the difference sits inside variance. Written up as it stands: the isolated win is measured, its end-to-end value on these shapes is not demonstrated. That is a fair thing to weigh against the change's complexity, and I would rather you weigh it than not see it.

So on your framing — that -66% in isolation was the only number backing the headline of the PR — that is still broadly true, and now visibly so rather than by omission. The full-pipeline sections higher in this file remain un-rerun; that suite does run the assembled enterprise output and does need a licence.

[claude]

The default invocation of this harness (no -iceberg.throughput.codec) will always fail this assertion.

The table here is created through the Iceberg Go library via client.CreateTable, and this PR's own docs state that such a table comes back carrying write.parquet.compression-codec: zstd materialised at creation — see docs/benchmark-results/iceberg.md ("The uncompressed rows above required setting the property explicitly") and the same note in iceberg.adoc.

Since an unset parquet.compression defers to the table property, resolveParquetCompression will pick zstd, writtenCodec will be ZSTD, and wantCodec is hard-coded to UNCOMPRESSED whenever the flag is empty — so the run aborts before reporting anything. The flag's own help text says "empty leaves it unset", which reads as "whatever the table does", not "expect uncompressed".

Suggested fix: derive the expectation from what the table actually resolves to (the properties already loaded and logged as TABLEPROPS a few lines above) rather than from the flag value, or make the flag default to an explicit codec so the unset case is never exercised.

[Jeffail]

Right on both counts, and slightly embarrassing: I hit this failure, correctly diagnosed the library behaviour behind it, wrote that up in the docs you are quoting — and then never fixed the assertion that had exposed it. So the default invocation has been broken since I added it. Fixed in f82e5f16.

Of the two options you offer I took the first, deriving the expectation from the resolution rule: an explicit setting wins, otherwise the table property applies, otherwise uncompressed, with declined and unrecognised names falling back to uncompressed. I would rather not default the flag to an explicit codec, because "no field set, whatever the table says" is a real user configuration and the one most people will actually be in — it seems worth keeping reachable rather than engineering around.

One deliberate choice worth flagging: I restated that rule in the test rather than calling resolveParquetCompression. It is unexported and in a different package, but even if it were reachable, asserting the implementation against itself would not prove much — a restatement at least fails if the two disagree.

I also went back to check whether this had contaminated the numbers, since a broken expectation next to a reported figure is exactly the sort of thing that should make a reader suspicious. It has not: 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 the published figures were measured with the assertion agreeing. The runs it would have aborted are the ones that produced nothing. Verified the default invocation and all three explicit codecs after the fix.

Also took your point about the help text — "empty leaves it unset" did read as "expect uncompressed", so it now says what unset actually resolves to and notes that the library materialises zstd at table creation.

[claude]

TestWriteThroughput lives in the integration package and stands up Docker containers via setupTestInfra, but it does not call integration.CheckSkip(t). Every other test in this package opens with it — e.g. integration_test.go#L23-L27 — and setupTestInfra itself does not perform the check (test_helpers.go#L54).

The -iceberg.throughput flag gate does keep it skipped by default, so this is a convention gap rather than a live failure, but the project test patterns state that integration.CheckSkip(t) belongs at the start of every integration test function ("Do not use build tags. Use integration.CheckSkip(t) at the start of every integration test function"). Suggested fix: add integration.CheckSkip(t) as the first statement, before the *throughputRun flag check, so the test honours the same environment gate as its siblings.