Shouldn't it be possible to assign field ids automatically? How does Iceberg do it?

Workflow [PR], commit [6322069]

Summary: ❌

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds Parquet field_id controls for Iceberg compatibility: explicit per-path overrides, sequential auto-assignment, nested-field handling, docs, and tests. The core writer changes look consistent with the current schema-building logic, but one claimed regression test still does not exercise the datalake guard it is supposed to cover, so the review remains blocked on that gap.

Findings

⚠️ Majors

• [tests/integration/test_storage_iceberg_no_spark/test_writes_reject_field_id_settings.py:50-61] The new Iceberg regression test still does not reach ParquetBlockOutputFormat.cpp:195. Engine-backed Iceberg writes freeze FormatSettings at CREATE TABLE time and ignore per-INSERT session/user format settings, so these two inserts bypass the guard entirely instead of proving that datalake metadata remains authoritative for field_ids. Rewrite the test to put the setting into CREATE TABLE ... SETTINGS ... / additional_settings, or route the write through INSERT INTO FUNCTION icebergLocal(...), where query-time format settings are actually used.

Final Verdict

Request changes. The writer logic is close, but the remaining regression test for the datalake guard is still testing the wrong surface and does not prove the invariant it claims to cover.

Merged the latest master (the branch was ~306 commits behind, 0 conflicts — the only file conflict, Mutations.cpp arm_tidy init-vars, was already fixed identically on master via #106102).

Renumbered the test 04309_parquet_column_field_ids → 04321_parquet_column_field_ids: the master merge brought in 04309_explain_insert_format, which collided with the 04309_ prefix (04321 is the next free number). Updated the internal temporary-file names accordingly.

Rebuilt locally (clean) and ran 04321_parquet_column_field_ids against the freshly-built binary — exact reference match (explicit overrides, auto-assign, mixed mode, default, nested Array/Map/Tuple auto-assign, dotted nested overrides, GeoParquet WKB collapse, and all the BAD_ARGUMENTS error cases). All review threads are resolved.

The CI reds are all unrelated to this Parquet-output-only change: the Performance Comparison failures (rand, logical_functions_small, async_remote_read) are flagged on both ::old and ::new, i.e. measurement noise, not a regression.

Merged latest master to refresh the stale base (the CI red was entirely 0s aborted jobs from the outdated merge base, not real failures). Clean merge, no conflicts; test 04321_parquet_column_field_ids does not collide with anything new on master.

Rebuilt clickhouse (clean) and re-ran the test against a fresh server — passes:

[1 / 1] 04321_parquet_column_field_ids: [ OK ] 4.79 sec.

All 6 review threads are resolved. CI should now run on a current base.

Ok. I understand why manual assignment is needed.
But I don't like ad-hoc invented substream names.

If we make substream names exactly as they are in ClickHouse (so we don't invent anything for this task), it will be good.

Apparently, the substream names are this way in Parquet :(

Pushed 9cd4798c24c..d582ec045d9:

• Merged latest master (the branch was ~994 commits behind and CONFLICTING). The merge was clean — no conflicts. The test 04321_parquet_column_field_ids does not collide with anything new on master (the 04321 numeric prefix is shared by several unrelated tests, but the full filename is unique), and both new settings remain correctly registered under the current dev 26.7 block in SettingsChangesHistory.cpp. Re-ran -fsyntax-only on ParquetBlockOutputFormat.cpp, PrepareForWrite.cpp, and FormatFactory.cpp against the merged tree — all clean.
• Documented the explicit-map full-coverage rule (d582ec045d9): the output_format_parquet_column_field_ids docs row now states that, with output_format_parquet_auto_assign_field_ids disabled, a non-empty map must cover every output column and every nested field (otherwise BAD_ARGUMENTS), and that auto-assign fills the remaining paths. Resolves the open docs thread.
• Updated the stale PR description: it referenced the old 04080_parquet_column_field_ids.sql and a non-existent "Arrow encoder path"; it now points to 04321_parquet_column_field_ids.sh with the actual coverage, and uses Closes: per convention.

The two remaining AI-review findings on the datalake path are addressed in their threads:

• The dotted-path collision concern on the datalake metadata mapping (getStorageColumnEncoding() → convertSchema) is pre-existing behavior on master — this PR does not touch that path; it only adds the user-setting path with its own collision check.
• The missing regression coverage for the datalake guard is not reachable from a stateless test (the column_mapper is only present for actual Iceberg/datalake writes); flagged for a follow-up integration test.

The previous CI reds are unrelated, known master-wide flakes (not touched by this Parquet-output change):

• Stress test (arm_ubsan): Logical error: 'Unexpected exception in refresh scheduling' in RefreshTask::doScheduling — the chronic RefreshTask flake (#106737 family), 328 failures across 217 PRs (37 on master) in the last 30 days.
• Upgrade check (amd_release): PostgreSQLConnectionPool connection to 192.0.2.1 (a non-routable TEST-NET-1 address) timing out in DatabasePostgreSQL::removeOutdatedTables() — the recurring upgrade-check PostgreSQL/MySQL connection flake.

CI re-runs on the fresh base after this push.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=101783&sha=latest&name_0=PR

Pushed d582ec045d9..f44537142bb.

Added the datalake-guard regression test that the AI Review "Request changes" verdict (and the ParquetBlockOutputFormat.cpp:195 thread) asked for — implementing the follow-up integration test mentioned in that thread.

New test tests/integration/test_storage_iceberg_no_spark/test_writes_reject_field_id_settings.py:

• Writes to an Iceberg table — the only path that hands ParquetBlockOutputFormat a non-null format_filter_info->column_mapper (via MultipleFileWriter::startNewFile, which builds the mapper from the Iceberg schema).
• Asserts that an INSERT with output_format_parquet_auto_assign_field_ids = 1, and one with output_format_parquet_column_field_ids = {'x': '1'}, each fail with BAD_ARGUMENTS (... cannot be used when writing to a datalake table that provides its own column-id mapping).
• Confirms a plain INSERT still succeeds and that the rejected inserts commit no rows.

This closes the gap the stateless 04321_parquet_column_field_ids test cannot reach (it writes via file / clickhouse-local, which never has a column_mapper), so dropping the guard would now turn CI red instead of leaving it green. I used the no_spark suite because allow_insert_into_iceberg is enabled cluster-wide there and the table is created and written entirely by ClickHouse (no Spark dependency), matching test_writes_multiple_files.py.

CI: the only red on the prior commit was Unit tests (msan, function_prop_fuzzer) → FunctionsStress.stress on SELECT reinterpret(CAST(-1012.7 AS Decimal(9, 1)), CAST('Decimal32(4)' AS String)) — the random-function property fuzzer, tracked by #107242, unrelated to this Parquet-output-only change.

The remaining open threads are maintainer design calls (the manual-assign rationale; the substream-name notation, which matches Parquet's own; and the pre-existing dotted-path behavior on the datalake metadata path) — left for @alexey-milovidov.

Heads-up on the two CI reds on f4453714 — neither is a Parquet-encoding regression.

1. The new datalake-guard test test_writes_reject_field_id_settings is red, and it stays red with the guard in place — so the assumption in my previous comment ("dropping the guard would now turn CI red instead of leaving it green") is actually inverted. The guard at ParquetBlockOutputFormat.cpp:195 never runs here, because the per-INSERT field-id settings never reach the Iceberg write path:

• create_iceberg_table("local", …) produces a CREATE TABLE … ENGINE=IcebergLocal(…). For a storage engine, format_settings is frozen at table-creation time from the CREATE … SETTINGS clause plus global defaults — "Use format settings from global server context + settings from the SETTINGS clause of the create query. Settings from current session and user are ignored." (registerStorageObjectStorage.cpp:54-70).
• The write path passes that frozen member straight through (StorageObjectStorage::write, StorageObjectStorage.cpp:612); unlike the read path it does not re-derive from the query context, and getOutputFormatParallelIfPossible then uses it verbatim because it is non-nullopt.
• So output_format_parquet_auto_assign_field_ids / output_format_parquet_column_field_ids set on the INSERT never land in format_settings.parquet. user_set_field_ids is false, the guard is skipped, the INSERT succeeds, and query_and_get_error raises "Client expected to be failed but succeeded!".

The practical consequence is larger than the test: these settings can only reach an Iceberg write through the table's CREATE … SETTINGS clause (or a profile/global default), never per-query. So the "session setting silently overrides the datalake field_id mapping" scenario the guard defends against cannot happen via session settings at all — and if the settings are baked into the table, the guard would fire on every write to it.

The fix direction depends on the design call in the open threads:

• keep the guard → the test should set the field-id settings in CREATE TABLE … SETTINGS (a separate table from the plain-insert one), where they reach the format and the write is rejected;
• drop the manual-assign path / always auto-assign → the guard and this test go away with it.

I left it rather than rewrite the test, since it's tied to that decision rather than a mechanical fix.

2. Unit tests (asan_ubsan, function_prop_fuzzer) → FunctionsStress.stress is the unrelated master-wide property-fuzzer failure tracked by #107242 (here it surfaced a reinterpret(CAST(… AS Decimal(9, 7)), 'Decimal32(5)') scale mismatch — function returned column of unexpected type or scale: Const(Decimal32) instead of Decimal(9, 5)). It fails on master (pull_request_number = 0) and ~25 other PRs the same day; nothing to do with Parquet output.

Merged master into the branch (f44537142bb..63220697583) — clean auto-merge, the net diff is unchanged (still the same 23 files, +681/-5; the two SettingsChangesHistory.cpp / FormatFactorySettings.h entries survived). This clears the stale CONFLICTING state.

The only PR-caused red is the new test_writes_reject_field_id_settings.py (it fails deterministically across every integration shard — flaky, amd_tsan, amd_msan, arm_binary, …). The other reds are unrelated: FunctionsStress.stress (#107242), Hung check (#107941), arm_release Unexpected return type decorrelation flake, and the AST fuzzer one is a heap-buffer-overflow in AggregateFunctionVarianceSimple::add — an aggregate-function fuzzer find, nothing to do with Parquet output.

Root cause of the test failure — the guard is unreachable on the path the test uses. The test creates a table engine (create_iceberg_table → ENGINE = IcebergLocal(...), no SETTINGS clause) and then does INSERT ... SETTINGS output_format_parquet_auto_assign_field_ids = 1. But for object-storage engines the FormatSettings are frozen at CREATE TABLE time — registerStorageObjectStorage.cpp is explicit: "Use format settings from global server context + settings from the SETTINGS clause of the create query. Settings from current session and user are ignored." So the per-INSERT setting never reaches the writer: in the ParquetBlockOutputFormat ctor user_set_field_ids is false, the has_metadata_mapping && user_set_field_ids guard doesn't fire, the INSERT succeeds, and query_and_get_error reports "Client expected to be failed but succeeded!".

A corollary that's directly relevant to the still-open manual-assign discussion: on the natural SET …; INSERT INTO <iceberg table> path these settings are silently (and safely) ignored — the Iceberg column_mapper is always authoritative there — so there is nothing to corrupt on that path, and dropping the guard would not turn this test red (it's red today with the guard present).

Two ways to make the test actually exercise the guard, if we keep it:

• Freeze the setting into the engine: CREATE TABLE t (...) ENGINE = IcebergLocal(...) SETTINGS output_format_parquet_auto_assign_field_ids = 1 (then any INSERT trips it — needs a separate table from the plain-insert case). The DataLake settings collection embeds format settings (that's the M(CLASS_NAME, Map) line this PR adds to DataLakeStorageSettings.h).
• Or write via the table function INSERT INTO FUNCTION icebergLocal(...) SETTINGS output_format_parquet_auto_assign_field_ids = 1 SELECT ... — table functions pass format_settings = std::nullopt, so they resolve from the query context at write time and the guard does fire.

I left the fix to you: whether to keep the guard at all is entangled with the manual-assign notation thread you're still deciding, and I couldn't run the integration suite in this environment to confirm which of the two rewrites is cleanest for the no_spark suite.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 144/176 (81.82%) · Uncovered code

Full report · Diff report