Write Iceberg `optional` lists, maps and structs as OPTIONAL in the Parquet footer by groeneai · Pull Request #112669 · ClickHouse/ClickHouse

groeneai · 2026-07-30T21:41:11Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

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

Description

An Iceberg field declared "required": false whose type is a container was written to Parquet as
REQUIRED, so the footer contradicted the Iceberg schema published by the same commit. The
optionality is not recoverable from the ClickHouse type: Array and Map are never wrapped in
Nullable (canBeInsideNullable() is false for both) and the Iceberg schema builder returns a
bare container, so prepareColumnArray / prepareColumnTuple / prepareColumnMap each
hard-coded their group REQUIRED. ColumnMapper already carries the recovered bit
(iceberg_optional_paths) and the ORC writer already consumes it; the Parquet writer threaded only
the field-id map.

This is deliberately not a schema-only flip: the reader takes its max definition level from the
schema while the writer takes the level bit width from the state built on the data path, so the two
have to move together or the file becomes unreadable. Both now receive the same IcebergOptionality
(convertSchema, plus the serial and parallel encoder paths). When a container's dotted Iceberg path
is marked optional, the group becomes OPTIONAL and one definition level is added below it, strictly
after updateRepDefLevelsForArray so a present-but-empty container keeps level 1 and level 0 stays
free to mean "null container". With no mapper, or a mapper carrying only field ids, the writer keeps
its current behaviour, which is what Iceberg position-delete writes rely on.

Validation: a new stateless test asserts, through pyarrow across both encoders, the per-leaf
definition levels and which schema node carries the OPTIONAL, with a required-container half as the
control. Without the fix its reference differs by 64 lines. Map keys, every repeated level and
optional scalars are asserted unchanged, and values round-trip identically, so this only corrects
metadata. No new or changed setting, so no SettingsChangesHistory.cpp entry; files written by
older versions keep their REQUIRED groups and stay readable.

Version info

Merged into: 26.8.1.491 (included in 26.8 and later)

An Iceberg field declared "required": false whose type is a complex container (list/map/struct) was serialized to Parquet with FieldRepetitionType::REQUIRED, so the footer contradicted the Iceberg schema published by the same commit. Spec-compliant external readers (Spark, Trino, pyiceberg) reject that. The optionality is not recoverable from the ClickHouse type: DataTypeArray and DataTypeMap both report canBeInsideNullable() == false, and the Iceberg schema builder returns a bare container while applying makeNullable only to leaf scalars. So prepareColumnArray, prepareColumnTuple and prepareColumnMap each hard-coded their group REQUIRED, and the only producer of OPTIONAL, prepareColumnNullable, is unreachable for them. ColumnMapper already carries the recovered bit as iceberg_optional_paths and the ORC writer already consumes it; the Parquet writer threaded only the field-id map. This cannot be a schema-only flip. The reader derives its max definition level purely from the schema while the writer derives the RLE level bit width from the state the data path builds, so raising the schema's OPTIONAL count alone desynchronizes them and the file becomes structurally unreadable. Both paths are now given the same IcebergOptionality: the schema path through convertSchema, and both the serial and the parallel encoder path in ParquetBlockOutputFormat. When a container's dotted Iceberg path is marked optional the group becomes OPTIONAL and one definition level is added below it, strictly after updateRepDefLevelsForArray: that function encodes an empty container as level 0, and level 0 under an OPTIONAL ancestor means "null container" to a reader, so incrementing afterwards shifts empty-but-present to 1 and leaves 0 free. A Nullable that already supplies the OPTIONAL level for the same path suppresses the extra one, since Nullable is transparent in Iceberg field naming. With no mapper, or a mapper carrying only field ids, the writer keeps its current behavior, which Iceberg position-delete writes rely on. The new stateless test asserts per-leaf definition levels through pyarrow across both encoders, with a required-container half as the control. pyarrow is the oracle rather than a ClickHouse round-trip because ClickHouse's reader normalizes a null container to an empty one, so a round-trip cannot see the ordering error the increment position guards against. Map keys, every repeated level and optional scalars are asserted unchanged and the values round-trip identically, so this only corrects metadata. No new or changed setting, so no SettingsChangesHistory.cpp entry. Files written by older versions keep their REQUIRED groups and stay readable. Related: ClickHouse#111775 Related: ClickHouse#109994 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…test A leaf's max definition level counts every nullable ancestor along its whole chain, so it cannot say WHICH ancestor contributed it. Moving the OPTIONAL from a container group down onto a node beneath it leaves both of the probe's observables byte-identical, which means the reference could not distinguish the intended footer from a plausible mis-implementation. Which node carries the OPTIONAL is the property an external Iceberg reader validates against the published schema, so it is exactly what the test has to pin. Add a per-node oracle built on pyarrow's typed schema_arrow field nullability, covering the nodes the change claims to flip (arr, m, st, nst.element, nq.inner) and the ones it claims not to (the map key, which is always required per the Iceberg spec, nst itself, and the optional scalar). The existing per-leaf level block and the decoded values stay untouched: schema_arrow collapses the repeated list wrapper, so it reports neither the repetition levels nor the present-but-empty versus null distinction. Emit the probes from a fixed list rather than by iterating a mapping so the reference cannot churn on iteration order, and prefer schema_arrow over the raw footer repr, which embeds field ids and is an unversioned pretty-printer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ader claims The test header claimed spec-compliant external readers (Spark, Trino, pyiceberg) reject a REQUIRED group where the Iceberg schema says optional. That is unverified for Spark and Trino, and false for pyiceberg: Iceberg documents makeColumnOptional as supported schema evolution, so the lenient direction our old footer sat in is accepted by the reference implementation's own schema-compatibility validator. Replace it with the harm that is actually measurable. A REQUIRED group cannot encode a null container at all, so writing a null list, map or struct silently read it back as an empty one; with the group marked OPTIONAL the null survives. Verified on pyarrow 22.0.0 and 24.0.0 for all three container kinds. That is a better reason for the change than a rejection that cannot be demonstrated in this job, whose image pins pyarrow only. The probe commentary said which node carries the OPTIONAL is what an external reader "validates"; softened to what has to match the published schema, for the same reason. Comments only. No source, no reference, no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The header claimed a REQUIRED group cannot encode a null container, so a null list/map/struct was silently read back as an empty one. That harm is not what this change fixes, and the reference shipped in the same commit refutes it. Every node this fix newly flips to OPTIONAL is a type that cannot hold a ClickHouse null: Nullable(Array(Int32)) and Nullable(Map(String,Int32)) are rejected at type construction because canBeInsideNullable() is false, and st, nst.element and nq.inner are not declared Nullable. The one nullable container, nq, was already emitted as an optional group by prepareColumnNullable before this change; the fix flips nq.inner. The test does insert NULL into ns and nq, both are optional groups in half A and half B, and their null row reads back as a struct of defaults in both halves on both encoders, while the scalar control sc reads back NULL. The header now states only what the node lines measure: the footer contradicting the Iceberg schema published by the same commit. Comment-only. No source, reference or behaviour change.

groeneai · 2026-07-30T21:41:13Z

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

Pre-publication review by an independent model, run cold against the resulting code before the
author's evidence was opened. Four rounds: 3 Gate B passes (engine codex) plus my own review each
round; 2 findings from the gate and 23 from my own review; 3 blocker/major findings AGREED and
fixed across 3 fix rounds; 0 DISAGREE. The final round is clean on both sides: Gate B
findings=0, own review 0 blockers and 0 majors.

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	The probe reported only each leaf's aggregate max definition/repetition level, so a REQUIRED container with an incorrectly OPTIONAL descendant could produce identical maxima and values. Asked for direct per-node assertions on `arr`, `m`, `st`, `nst.element`, `nq.inner`, plus confirmation that the map key stays required and the repeated wrapper stays repeated.	AGREE, fixed @ `e8af525`	Added a per-node `pyarrow` oracle reporting which schema node carries the OPTIONAL. The reference now carries 44 `node` lines covering all five named nodes plus `node m.key optional=False` and every unchanged `maxrep`. Verified load-bearing: it is the only assertion in the test that fires against a level-coherent mutation that moves the OPTIONAL one node down while keeping the file readable.
2	❌	The test header and the changelog entry both asserted that a REQUIRED group cannot encode a null container, so a null list/map/struct was silently read back as empty. This PR neither fixes nor can demonstrate that harm. Root cause was my own round-2 fix plan, not the author's execution.	AGREE, fixed @ `b7d3613`	Measured three ways: `Nullable(Array)` and `Nullable(Map)` are rejected with `ILLEGAL_TYPE_OF_ARGUMENT`, so no null is reachable on those columns; half A already showed the one `Nullable(Tuple)` as OPTIONAL before the fix; and the inserted NULL row reads back identically in both halves on both encoders. Wording now states the measured footer-versus-schema contradiction only. Verified discharged: 0 hits for the refuted claim in both the test and the PR body.
3	❌	The changelog category was `Bug Fix (user-visible misbehavior in an official stable release)`.	AGREE, fixed @ `0d94de1`	Iceberg writing is gated behind `allow_insert_into_iceberg`, tier BETA, default false (`Settings.cpp:8441`), so this is not reachable in a default stable release. The merged ORC sibling #111775 shipped the identical mechanism under `Not for changelog`. Category switched to match, byte-identically to the live upstream template line.
4	💡	The `pyarrow` probe proves a footer mismatch, not reader rejection, and Iceberg's own `makeColumnOptional` schema evolution makes the lenient direction acceptable.	AGREE, fixed @ `0d94de1`	Independently confirmed by driving `pyiceberg` 0.11.1 `_check_pyarrow_schema_compatible`: a required list against an optional request is ACCEPTED. All reader-rejection wording removed from the test header; it was never in the PR body.

Severity: ❌ blocker / ⚠️ major / 💡 nit.

Independent verification I performed before adjudicating, recorded because it is what the verdicts
rest on. I enumerated the invariant carriers from source before reading the author's plan, and my
list matched it with no divergence: the three hard-coded REQUIRED group sites are exactly
prepareColumnTuple / prepareColumnArray / prepareColumnMap (found via the setter spelling
__set_repetition_type, since the assignment form finds only a comparison and the Nullable wrapper
and misses all three); max_def has exactly three increment sites; the reader derives its max
definition level purely from the schema (Reader.cpp:1863 into SchemaConverter.cpp:260-281, whose
chassert(level.def == levels.size()) makes the coupling explicit), which is why a schema-only flip
would desynchronize writer and reader; there is exactly one registered Parquet output format and
exactly three prepareColumnForWrite / convertSchema call sites, all three patched; and a
container can never reach the LowCardinality route because DataTypeTuple overrides only
canBeInsideNullable.

Three points I checked hardest. The dotted-path root is identical across all three call sites
(c.name, header.getByPosition(i).name, and task.column_name assigned from the same
expression), and below the root the producer and the consumer both use Nested::concatenateName
with identical key / value / element tokens, so schema and data cannot disagree per column.
The new definition-level helper runs strictly after updateRepDefLevelsForArray at both container
sites, which is load-bearing: that function encodes an empty array as level 0, and level 0 under an
OPTIONAL ancestor reads back as a null container, so incrementing afterwards shifts
empty-but-present to 1 and leaves 0 free for null. Raising max_def does not regress the three
downstream max_def == 1 special cases: null_count correctly stops being emitted when two
nullable ancestors exist, which is the same behaviour Nullable(Tuple(Nullable(x))) already
produces on master, and the definition-level histogram is sized from the final max_def after all
preparation, so it cannot go out of bounds.

Test liveness was traced rather than asserted. Against a pristine build the reference differs by 64
changed content lines, 36 leaf and 28 node, with 0 data lines, so the fix moves metadata only.
Half A is a genuine control that a revert leaves unchanged, so an unconditional flip would redden
it. pyarrow rather than a ClickHouse round-trip is the ordering oracle because ClickHouse's own
reader normalizes a null container to an empty one, so a round-trip cannot see that distinction.
Both encoder paths are exercised, which matters because the serial and parallel paths build the
state independently and a fix applied to only one would produce a footer that disagrees with the
data.

Session id: cron:clickhouse-review-slot-10:20260730-210300

groeneai · 2026-07-30T21:42:19Z

Pre-PR validation gate (a-i)

#	Question	Answer
a	Deterministic repro?	Yes. One `INSERT` into an `IcebergLocal` table whose metadata marks the complex fields `"required": false`, then read the Parquet footer. No randomization, no timing, no sanitizer. `tests/queries/0_stateless/04653_iceberg_parquet_optional_complex.sh`.
b	Root cause explained?	Container optionality is not recoverable from the ClickHouse type (`Array`/`Map` are never `Nullable`, and the Iceberg schema builder returns a bare container), so the three container preparers hard-coded their group `REQUIRED`. The recovered bit already exists in `ColumnMapper` as `iceberg_optional_paths` and is already consumed by the ORC writer; the Parquet writer threaded only the field-id map.
c	Fix matches root cause?	Yes. It delivers the already-recovered metadata to the two places that need it and adds the definition level the new schema level implies. No widened bounds, no `no-random-*` tag, no defensive check at a failure site.
d	Test intent preserved / new test added?	New stateless test added; no existing test weakened or removed.
e	Both directions?	Yes, on binaries built in this worktree: pristine (merge-base sources, build id `5dfab0d96969`) FAILs the test with a 92-line reference diff (64 changed lines: 36 leaf-level, 28 per-node); fixed (`67f2be2a40fe`) passes. Each arm asserts `readelf` build id == `SELECT buildId()`, and restoring the fix relinks bit-identically. Eight mutations of the fix each fail the oracle they target, including one that keeps the schema flip but drops the definition level and makes the file structurally unreadable, and one that moves the OPTIONAL a level down while keeping the decoded values intact, which only the per-node assertions catch.
f	General across CODE paths?	One shared helper applied at all three container preparers, and the same `IcebergOptionality` fed to the schema path and to BOTH the serial and parallel encoder paths, which the test exercises via `output_format_parquet_parallel_encoding` 0 and 1. A fix applied to only one data path silently produces unreadable files. Sibling writers: ORC fixed in #111775, Avro tracked separately (one concern per PR). Exactly one registered Parquet output format.
g	General across INPUTS?	Optional `struct`, `list`, `map`, a nested optional element inside a required list, and a container below a `Nullable`-owned struct are all fixed and asserted. Asserted unchanged: map keys (always required per the Iceberg spec), every `repeated` list/`key_value` level, optional leaf scalars, non-Iceberg writes (gated on the mapper) and position-delete writes. `LowCardinality` has no container route: `DataTypeTuple` does not override `canBeInsideLowCardinality()`, so `LowCardinality(Nullable(Tuple))` throws at construction. Boundary values in the fixture: empty list, empty map, NULL scalar, NULL struct.
h	Backward compatible?	Yes. No new or changed setting, so no `SettingsChangesHistory.cpp` entry (verified: the diff touches nothing under `src/Core/`). Files written by older versions keep their `REQUIRED` groups and stay readable. Newly written files become spec-compliant, matching what other engines write.
i	Invariants preserved?	The writer/reader definition-level agreement is the invariant at stake and both paths now move together. `max_def` overflow is guarded by the existing check before every increment, and `def` is materialized on the 0 to 1 transition so the "def is empty iff max_def == 0" contract holds. The context is filled in the constructor and read-only afterwards, so the encoder threads share it without synchronization, matching how `format_filter_info` is already shared. No new allocation on the data path.

Regression sweep: Parquet + Iceberg stateless selection, identical selection and config on both
binaries, compared over the 38 tests that completed in both arms: 0 regressions, and the only
difference is the new test itself. 50 randomized runs of the new test, randomization left on:
50 OK / 0 FAIL, no tag needed.

Session id: cron:clickhouse-review-slot-10:20260730-210300

groeneai · 2026-07-30T21:46:18Z

cc @PedroTadim @vitlibar, could you review this? It is the Parquet half of the same optional-complex defect you merged for ORC in #111775: an Iceberg optional list/map/struct was written as REQUIRED, since Array/Map are never wrapped in Nullable so the optionality is only recoverable from the per-path Iceberg metadata that the Parquet writer did not consult. The schema and both encoder data paths now receive it together, because the reader derives its max definition level from the schema while the writer derives the level bit width from the data path.

clickhouse-gh · 2026-07-30T23:12:53Z

Workflow [PR], commit [b7d3613]

Summary: ❌

job_name	test_name	status	info	comment
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel)		FAIL
	Server died	FAIL	cidb	IGNORED
	01660_join_or_any	UNKNOWN	cidb	IGNORED
	04328_reader_executor_kpi_async_metric	UNKNOWN	cidb	IGNORED
	04490_any_join_constant_condition_reorder	UNKNOWN	cidb	IGNORED
	Logical error: Got read request from replica A for unknown stream B. (STID: 5217-505b)	FAIL	cidb	IGNORED
Upgrade check (amd_release)		FAIL
	Error message in clickhouse-server.log (see upgrade_error_messages.txt)	FAIL	cidb	IGNORED

AI Review

Summary

This PR threads Iceberg per-path optionality into Parquet schema generation and both encoder paths so optional list, map, and struct groups are emitted as OPTIONAL instead of being derived only from ClickHouse Nullable-ness. The current diff, prior discussion, and added regression test are consistent with that contract; I did not find an unresolved correctness, compatibility, or evidence gap that warrants an inline review comment.

Final Verdict

Status: ✅ Approve

clickhouse-gh · 2026-07-31T03:18:02Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.50%	86.50%	+0.00%
Functions	91.90%	91.80%	-0.10%
Branches	78.70%	78.60%	-0.10%

Changed lines: Changed C/C++ lines covered: 129/130 (99.23%) · Uncovered code

Full report · Diff report

groeneai · 2026-07-31T04:25:38Z

CI finish ledger — `b7d3613`

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix
task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

CI is fully finished on this head: 174 check-runs, 0 queued or in progress, Finish Workflow
and Config Workflow both success, and more than 20 minutes have passed since the last check
completed.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / STID 5217-505b	logical error `Got read request from replica 1 for unknown stream ...`	#111689 (mine, open)
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / `Server died`	the shard death caused by the logical error above, same job, one event	#111689 (mine, open)
Upgrade check (amd_release) / `Error message in clickhouse-server.log`	`DiskLocalCheckThread` leaks `clickhouse_disk_checker_<uuid>` errors (Code 107 `FILE_DOESNT_EXIST` plus Code 458 `CANNOT_UNLINK`) into the log	a fix task is created (investigating at full effort, fixing-PR link to follow here)

Neither is caused by this PR, whose diff is confined to the Iceberg Parquet writer
(src/Processors/Formats/Impl/Parquet/**) plus its tests.

The two coverage rows are one event: the unknown stream logical error kills the shard, and
Server died is the job noticing. The signature has 205 hits across 169 unrelated pull requests
and 19 master hits over 30 days, first seen 2026-07-02, so it is a well-established trunk defect;
#111689 ("Fix unknown-stream LOGICAL_ERROR in parallel replicas with projection short-circuit")
is the fixing PR and is still open, so this head could not have carried it.

The upgrade-check row is the fleet-wide error-log leak on that job: 34 rows across 34 distinct
pull requests and 0 master rows over 30 days, all on Upgrade check (amd_release), roughly
one pull request per calendar day with exactly one row each. Every pull request running that
check picks it up, so this is a carrier, not an attribute of this diff.

Session id: cron:our-pr-ci-monitor:20260731-040000

groeneai and others added 4 commits July 30, 2026 17:55

alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 30, 2026

clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 30, 2026

PedroTadim self-assigned this Jul 31, 2026

PedroTadim added this pull request to the merge queue Jul 31, 2026

Merged via the queue into ClickHouse:master with commit e61186d Jul 31, 2026
175 of 178 checks passed

robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 31, 2026

alexey-milovidov mentioned this pull request Jul 31, 2026

Added Parquet Shredded VARIANT Support to ParquetReaderv3 #102499

Open

1 task

Navigation Menu

Uh oh!

Write Iceberg `optional` lists, maps and structs as OPTIONAL in the Parquet footer - #112669

Write Iceberg `optional` lists, maps and structs as OPTIONAL in the Parquet footer#112669
PedroTadim merged 4 commits into
ClickHouse:masterfrom
groeneai:fix-iceberg-parquet-optional-complex-groups

groeneai commented Jul 30, 2026 •

edited by robot-ch-test-poll4

Loading

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

clickhouse-gh Bot commented Jul 30, 2026 •

edited by PedroTadim

Loading

Uh oh!

clickhouse-gh Bot commented Jul 31, 2026

Uh oh!

groeneai commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

groeneai commented Jul 30, 2026 • edited by robot-ch-test-poll4 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Description

Version info

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

clickhouse-gh Bot commented Jul 30, 2026 • edited by PedroTadim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

clickhouse-gh Bot commented Jul 31, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jul 31, 2026

CI finish ledger — b7d3613

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

groeneai commented Jul 30, 2026 •

edited by robot-ch-test-poll4

Loading

clickhouse-gh Bot commented Jul 30, 2026 •

edited by PedroTadim

Loading

CI finish ledger — `b7d3613`