Fix missing Iceberg field-ids in ClickHouse-written manifest and manifest-list files

[groeneai]

Closes: #111763
Related: #109994

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix Iceberg tables written by ClickHouse being unreadable by external readers (PyIceberg, Spark). The Avro schemas embedded in the manifest-list and manifest files omitted the Iceberg field-id properties, so scan planning failed with Cannot convert field, missing field-id.

Description

The bundled avro-cpp JSON schema compiler discards the Iceberg field-id / element-id attributes, so the schema it re-serialized into the avro.schema header of a written manifest / manifest-list omitted them. ClickHouse reads its own manifests via the Iceberg schema metadata key so did not notice, but PyIceberg and Spark read the Avro header schema during scan planning and reject a schema whose fields lack field-id (ValueError: Cannot convert field, missing field-id) - before ever opening the (correctly written) data file.

Fix: in generateManifestFile and generateManifestList, write the original id-carrying JSON schema string (the same one used to build the writer) as the avro.schema header, instead of the id-stripped one the avro-cpp compiler produces. This is spec-conformant (Iceberg mandates fixed field-ids like manifest_path=500, status=0, data_file=2, and the manifest-entry map key/value ids) and matches the encoded record layout exactly; the encoded data bytes are unchanged.

This also fixes a latent partition-struct field-id mismatch the change would otherwise expose: extendSchemaForPartitions hardcoded the manifest partition field-id as 1000 + i, but ClickHouse numbers partition-spec fields from 1001. Iceberg projects manifest partition values onto the spec by field-id, so once the id becomes visible the mismatch would break external readers. The id is now derived from the persisted partition spec, falling back to the 1000 + i sequential default only for legacy v1 specs that do not track partition field-ids.

Regression test (test_writes_manifest_field_ids_spark_read) asserts the emitted manifest-list and manifest schemas carry the required field-ids and that Spark can plan and read a ClickHouse-written table. A ClickHouse round-trip cannot catch this (ClickHouse reads via the Iceberg schema key, not the Avro field-ids), so the test is grounded in an external reader.

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. clickhouse local IcebergLocal write of a 2-row table, then decode the manifest-list (snap-*.avro) and manifest (*.avro) avro.schema header with avro-python: pristine HEAD = 92 missing field-id/element-id (partitioned), 61 (non-partitioned). 100% reproducible, no randomization.
b	Root cause explained?	Yes. The bundled avro-cpp JSON compiler (Compiler.cc) parses only name/type/default/doc and drops the Iceberg field-id/element-id attributes, never populating the write-side setFieldIds/setElementId. So DataFileWriterBase::init() serializes the id-less compiled schema into the avro.schema header. PyIceberg/Spark read that header during scan planning and reject fields lacking field-id. ClickHouse reads via the Iceberg schema metadata key so never noticed.
c	Fix matches root cause?	Yes. Override the avro.schema header with the original id-carrying JSON (schema_representation, the same string used to build the writer) in generateManifestFile + generateManifestList. Directly addresses the dropped-attributes mechanism; encoded data unchanged.
d	Test intent preserved / new tests added?	New test_writes_manifest_field_ids_spark_read in the existing #109988 Spark field-id test file: asserts manifest-list + manifest header field-ids (manifest_path=500, status=0, data_file=2, map key/value=117/118, partition id==spec id) AND that Spark plans+reads the table. External reader required — a ClickHouse round-trip cannot observe this.
e	Both directions demonstrated?	Yes. Baseline (Build 811d9ad0) = 92 missing ids FAILS; fixed (Build b0805047) = 0 missing PASSES. A/B via patch-file revert+rebuild. Test field-id assertions dry-run against real fixed-binary output = pass.
f	Fix is general across code paths?	Yes. Both manifest-Avro writer sites covered (generateManifestFile, generateManifestList) — confirmed the only two DataFileWriter sites for these files. Not a symptom guard: it fixes the schema-serialization source. Also fixed the exposed partition-id mismatch at its source (extendSchemaForPartitions).
g	Fix generalizes across inputs?	Yes, verified matrix (all 0 missing ids): non-partitioned, single/multi-partition (ids match spec 1001/1002), v1 format, 3x append (multiple manifests), Nullable/Float64/Date/DateTime64. Map-logical arrays keep key/value ids (no spurious element-id, per spec); true lists keep element-id.
h	Backward compatible?	Yes. No settings change, no encoded-data format change (bytes identical; only the header schema now carries the field-id attributes it should per the Iceberg spec). ClickHouse's own reader unaffected; old readers ignore the extra Avro attributes.
i	Invariants and contracts preserved?	Yes. The avro.schema header now matches the record layout used for encoding. partition_spec->getArray(fields) is the same object already dereferenced a few lines later, so no new null-safety risk. One partition field emitted per column, in order; non-partitioned = empty partition struct (verified).

Session id: cron:clickhouse-author-slot-0:20260724-113800

[groeneai]

Internal second-model review — adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex; bounded: 1 full pass + 1 fix round + 1 delta recheck).

Full pass (sha e1708e9): 0 findings. Coverage: all changed files in full, every manifest-writer caller, sibling Avro schemas / avro-cpp writer internals, partition-spec generation, the partitioner, and the test fixtures/helpers. Delta recheck (sha fbb19f2, comment-only tightening): 0 findings.

No blockers, majors, or nits raised. Nothing to adjudicate.

Session id: cron:clickhouse-author-slot-0:20260724-113800

[groeneai]

cc @scanhex12 — could you review this? It completes the Iceberg field-id story on the metadata side: the avro-cpp JSON compiler drops the field-id/element-id attributes, so generateManifestFile/generateManifestList now write the original id-carrying JSON as the avro.schema header (and derive the manifest partition-struct id from the persisted spec). @PedroTadim requested this in #111763.

[clickhouse-gh]

Workflow [PR], commit [fbb19f2]

Summary: ❌

job_name	test_name	status	info	comment
Upgrade check (amd_release)		FAIL
	Error message in clickhouse-server.log (see upgrade_error_messages.txt)	FAIL	cidb	IGNORED

---

AI Review

Summary

This PR fixes the missing Iceberg field-id / element-id metadata in ClickHouse-written manifest-list and manifest Avro headers by overriding avro.schema with the original id-bearing JSON and by reusing persisted partition-spec ids when materializing the manifest partition struct. I checked the changed writer paths, the unchanged callers in insert, mutation, and manifest-compaction flows, the linked issue, and the added regression coverage; I did not find a remaining correctness issue in the current diff.

Final Verdict

Status: ✅ Approve

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.20%	86.20%	+0.00%
Functions	92.00%	92.00%	+0.00%
Branches	78.40%	78.30%	-0.10%

Changed lines: Changed C/C++ lines covered: 27/30 (90.00%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — fbb19f2

Every failure below has an owner. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Upgrade check (amd_release) / Error message in server.log: txn_version.txt Code 27 parse + intersects previous part Code 246 (part_loading_tree_rollback on object storage)	test-induced (frozen previous-release tests run without storage-skip tags)	fixing PR #111772 (ours, open)
Bugfix validation (integration tests, amd64/aarch64)	n/a	SUCCESS at check level (the per-test FAIL rows are the expected fails-on-master-HEAD validation markers)
CH Inc sync	-	CH Inc sync (private, not actionable)

No PR-caused failure: #111786's diff is IcebergWrites.cpp manifest/manifest-list field-id emission + integration test; it does not touch part loading, txn_version, or storage tags.