Fix Iceberg ARRAY columns with dot-separated names returning empty lists#1894
Fix Iceberg ARRAY columns with dot-separated names returning empty lists#1894il9ue wants to merge 7 commits into
Conversation
When querying an Iceberg table through the `iceberg(...)` table function or a DataLakeCatalog, a column whose name contains a `.` and whose type is `Array(T)` (e.g. `` `a.b` ARRAY<STRING> ``) returned empty arrays instead of the stored values. The same data read by Spark returned the expected values. Fixes ClickHouse#90731. The Parquet V3 reader path (`SchemaConverter` + `ColumnMapper` + `FormatFilterInfo`) is already correct after the dotted-name field-id work in 0a218cd, 4b733ba and f24c1a4. This change addresses two residual upstream defects that affect dotted-name `Array(T)` columns regardless of source: * `ColumnsDescription::getAllRegisteredNames` explicitly filtered out any column whose name contained `.`, under the assumption such names were always flattened Nested subcolumns. A column whose stored name literally contains a dot (allowed by MergeTree with backticks, and produced by Iceberg / Spark) is a first-class registered name and must appear in `IHints` misspelling suggestions. The function is only consumed by `IHints`-style suggestion paths (and by `StorageSystemZooKeeper` for column-name iteration, where no dotted names exist), so relaxing it has no effect on parsing, planning, storage, or wire protocol. * `NestedUtils::getSubcolumnsOfNested` treated every `Array(T)` column whose name contained `.` as a flattened element of a synthetic `Nested` structure named after the prefix. This caused the Arrow, ORC and pre-V3 Parquet readers to look for a struct field with the prefix name in the data file rather than the literal dotted column, returning an empty array. The fix uses a two-pass scan: a synthetic `Nested` entry is only emitted when at least two `Array(T)` columns share the same dotted prefix. A lone column such as `a.b: Array(T)` no longer appears in the synthetic-Nested map. Genuine flattened `Nested` with multiple fields is unaffected; the existing early-continue on `isNested()` also covers the one-field-Nested edge case. Tests: * `tests/integration/test_storage_iceberg_with_spark/test_column_names_with_dots.py::test_dotted_array_column` — end-to-end repro of ClickHouse#90731 against s3, azure and local storage. * `test_dotted_array_alongside_real_nested` in the same file — mixed- schema regression guard verifying a lone dotted `Array` column coexists with genuine flattened-Nested siblings. * `tests/queries/0_stateless/04259_dotted_array_not_nested.sql` — isolates Bug B without Iceberg. * `tests/queries/0_stateless/04260_dotted_column_in_hints.sh` — verifies Bug A by checking the misspelling hint output. Changelog category (leave one): - Bug Fix (user-visible misbehavior in an official stable release) Changelog entry: Fix reading Iceberg tables whose `ARRAY` column names contain a dot (e.g. `` `a.b` ARRAY<STRING> ``), which previously returned empty arrays. Two upstream defects were responsible: `ColumnsDescription::getAllRegisteredNames` filtered out dotted names, and `NestedUtils::getSubcolumnsOfNested` misclassified lone dotted `Array(T)` columns as flattened `Nested` children. (cherry picked from commit f8467af)
CI triage on ef84f17— the failures are unrelated to this change: Fast test (the only blocker in the PR workflow) reported The Community PR workflow's 25 "errors" are also unrelated — they're GitHub Actions This looks like a flaky |
|
In upstream Fast tests are failed too, with Looks like reason is a changes in And can't find |
There was a problem hiding this comment.
Two upstream defects were responsible: ColumnsDescription::getAllRegisteredNames filtered out dotted names, and NestedUtils::getSubcolumnsOfNested misclassified lone dotted Array(T) columns as flattened Nested children.
This part does not belong to changelog (both here and in your upstream PR). If you want, you can put it somewhere in description (however, I see, you already have more).
Changelog entry is a short user-facing line, one of tens of lines in release notes, it does not need all that implementatuon details.
mkmkme
left a comment
There was a problem hiding this comment.
The fast test is failing due to nullptr dereference. Please fix it
…s not needed The earlier `Nested::getSubcolumnsOfNested` change added a two-pass `>= 2` prefix-count gate so a lone dotted `Array(T)` column (e.g. `` `a.b` ``) would not be collapsed into a synthetic `Nested`. That gate is the root cause of a server crash and breaks existing mutation behavior, and it is unnecessary for the Iceberg fix. This restores `getSubcolumnsOfNested` to the `antalya-26.3` base behavior (it becomes byte-identical to base again). The `ColumnsDescription::getAllRegisteredNames` hint fix and the regression tests are kept. Root cause of the crash ======================= The gate's prefix count is computed over whatever column subset the caller passes. For wide parts, `IMergeTreeReader` builds `converted_requested_columns = Nested::convertToSubcolumns(columns_)` from the *per-read requested set*. When a single member of a genuine flat-`Nested` group is read on its own, the subset count is 1, so the gate refuses to remap it to a `Nested` subcolumn: * `03742_test_flattened_crash` (`SELECT arr.nested FROM ... ORDER BY arr.nested`): the dropped-and-re-added `arr.nested Array(Tuple(a String, b Float64))` is materialized through the plain-array default path instead of the `Nested` path, producing a column whose declared type does not match its backing data. Sending the result then dereferences a null `ColumnString` -> `SIGSEGV` in `SerializationString::serializeBinaryBulk` (via `SerializationArray` -> `SerializationTuple` -> `SerializationNamed` -> `NativeWriter`). The fault zeroes the test-failure count, which is why the Fast test reports `Server died` instead of a failed test. * `02565_update_empty_nested`: `nested.arr2` read alone during the mutation loses the shared-offset `Nested` serialization, so its default value reads with the wrong size -> `validateNestedArraySizes` reports `SIZES_OF_ARRAYS_DONT_MATCH` (2 vs 0). The count cannot be made reliable from the per-read subset: at the `NamesAndTypesList` level a real flat-`Nested` member and a standalone dotted `Array(T)` are indistinguishable, and basing the decision on the full storage schema instead introduces a separate mutation-write corruption (`CANNOT_READ_ALL_DATA`, truncated `nested.size0` marks). Why the gate is not needed ========================== The Iceberg / object-storage read path does not call `getSubcolumnsOfNested`, `collect`, or `convertToSubcolumns` at all (only MergeTree, Log, `generateRandom`, `mergeTreeIndex`, and the Arrow/ORC struct-prefix branch do, and a lone dotted column does not enter that branch). Reading a lone dotted `a.b Array(String)` — and a mix of a lone dotted column with a genuine flat `Nested` (`c.x`, `c.y`) — from a real local Iceberg table (both the `Iceberg` engine and the `icebergLocal` table function) returns the correct values without this change; the dotted-name `Array` read is already handled by the Parquet field-id work. So the gate provided no benefit while introducing the crash. Validated locally: `03742_test_flattened_crash` no longer crashes, `02565_update_empty_nested` returns `450000 450000`, `04259_dotted_array_not_nested` and `04260_dotted_column_in_hints` pass, the `icebergLocal` dotted-array reads are correct, and single-field `Nested` reads are unchanged.
|
Pushed a fix for the Fast test crash (commit The gate turns out not to be needed for the Iceberg fix (the object-storage read path never calls these functions; dotted CI is re-running now — watching Fast test and the Iceberg integration test. Full root-cause writeup once it's green. Note: upstream ClickHouse#105546 carries the identical gate, so it'll need the same revert to stay in sync. |
Status update on commit
|
Ready for Review
The crash is fixed. Fast test is green: Root cause: the Remaining red checks are not related to this change:
@mkmkme @ianton-ru @zvonand — could you take another look? Separately: upstream ClickHouse#105546 carries the identical gate (its Fast test fails the same way) and will need the same revert to stay in sync. |
|
@il9ue please do push the fixing commit to the upstream as well. Their AI review can spot some things we don't from the get-go. |
No, it wasn't 😄 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| names.reserve(columns.size()); | ||
| for (const auto & column : columns) | ||
| { | ||
| if (!column.name.contains('.')) |
There was a problem hiding this comment.
Now PR only remove this single line.
This code was added in ClickHouse#35852
Can this change break hints?
For example return all database.table combinations instead of only database, or something like this?
There was a problem hiding this comment.
If this check was added, it is possible it was added by some reason in that PR. Why?
There was a problem hiding this comment.
Looked into both — why the filter was there, and whether removing it breaks hints. Short answer: it's safe, and the database.table case can't arise here.
Why ClickHouse#35852 added it. That PR introduced ColumnsDescription::getAllRegisteredNames together with the dot-filter as part of a new feature (column-name hints). The filter was a design choice to keep flattened-Nested subcolumns (e.g. n.a) out of the suggestion list — not a guard added to fix a specific breakage. So removing it doesn't re-open a known bug; it only makes dotted / Nested-subcolumn names eligible as suggestions.
It's column-scoped — so no database.table. This method iterates one table's columns and returns each column.name. It only ever returns column names of a single table, never database or table names. The . in a.b is part of a literal column name, not a db.table separator, so no database.table combination can originate here.
No consumer splits on .. The list feeds only the IHints prompter (NamePrompter::getHints), which is pure Levenshtein edit-distance over whole candidate strings — it never tokenizes on .. The consumers are the "no such column" / ALTER MODIFY|RENAME|DROP hint messages, plus StorageSystemZooKeeper (whose columns are hardcoded and contain no dot, so the change is a no-op there). A dotted name is just one more intact candidate; it appears only if it's an edit-distance near-miss of the typed name.
Observed output (filter removed):
MODIFY `a.x` -> Maybe you meant: ['a.b'].
multi-dot `a.b.x` -> Maybe you meant: ['a.b.c']. (intact, not split)
Nested `n.x` -> Maybe you meant: ['n.a'].
non-dotted `idd` -> Maybe you meant: ['id']. (machinery unchanged)
far typo `xyz` -> (no suggestion) (never fabricates combos)
Multi-dot names stay whole; a far typo yields no suggestion. Removing the filter strictly adds correct near-match candidates and never malforms one. (I inferred the "before" from the single removed line plus the xyz no-match behavior rather than building a separate base binary — happy to produce a literal side-by-side if you'd like one.)
I also tightened 04260 to assert the dotted name appears specifically inside the Maybe you meant: [...] list, so if the filter were ever re-added (emptying that list) the test fails directly.
| -- A lone Array(T) column with a dot in its name must not be collapsed into | ||
| -- a synthetic Nested structure and must be readable as a plain array. | ||
|
|
||
| CREATE TABLE t1 (`a.b` Array(String)) ENGINE = Memory; |
There was a problem hiding this comment.
I am not sure this test makes sense in this PR. The PR is about Iceberg, and this test is for ENGINE=Memory
There was a problem hiding this comment.
valid point — you're right that 04259 didn't belong. It used ENGINE = Memory, which doesn't exercise the Iceberg read path and doesn't even route through Nested::convertToSubcolumns / collect, so it wouldn't have caught a regression in the reverted code either.
Replaced it with 04261_iceberg_dotted_array.sh, which reads dotted-name Array columns through the actual Iceberg path — the IcebergLocal engine plus the icebergLocal() table function with an explicitly-declared schema. It asserts:
- a lone
`a.b` Array(String)reads back its stored values (['x','y','z']) rather than the empty array of Iceberg: Selecting from ARRAY column with dot-separated name returns empty lists ClickHouse/ClickHouse#90731, and - a mixed table where the lone dotted column sits next to dotted columns sharing a prefix (
`c.x`,`c.y`) — all round-trip correctly.
That's the Iceberg-path coverage the PR was missing. Local-only (no Spark/S3), tagged no-fasttest, and stable across randomized settings and async_insert=1.
|
|
||
| $CLICKHOUSE_CLIENT -q " | ||
| CREATE TABLE t_dotted_hint (\`a.b\` Array(String)) | ||
| ENGINE = MergeTree ORDER BY tuple(); |
There was a problem hiding this comment.
Same with this test. It's testing MergeTree whilst the issue seemed to be for Iceberg specifically. Or am I getting it wrong?
There was a problem hiding this comment.
Valid point. The fix isn't in Iceberg code — the bug surfaced via Iceberg, but the root cause was in engine-agnostic machinery:
ColumnsDescription::getAllRegisteredNames(the dot-name filter) andNestedUtils. This test targets exactly the line this PR changes (the hint path for a dotted column name), and MergeTree is just the simplest engine to exercise it deterministically without a Spark+S3 setup.- It's also what answers @ianton-ru's question above — it pins the hint behavior for a dotted name so we don't silently regress Refactoring of hints for column descriptor ClickHouse/ClickHouse#35852. I'll make sure it asserts the actual hint output (not just readability) and link the before/after here.
There was a problem hiding this comment.
This also tells the missing hole of Iceberg test suite I've shared with you earlier and I will share my findings into our team channel
There was a problem hiding this comment.
The fix isn't in Iceberg-specific code — the bug surfaced via Iceberg, but the root cause was in engine-agnostic machinery: the dot-name filter in ColumnsDescription::getAllRegisteredNames. This test targets exactly the line this PR removes (the hint path for a dotted column name), and MergeTree is just the simplest engine to exercise it deterministically without a Spark+S3 setup. It's also what backs @ianton-ru's question above — it pins the hint behavior for a dotted name so re-adding the filter (which would empty the Maybe you meant: [...] list) fails the test directly. I tightened it to assert the dotted name appears specifically inside that suggestion list, not just anywhere in the message.
…tion list The test exercises the line PR #1894 removes (the dot-filter in `ColumnsDescription::getAllRegisteredNames`). It now asserts the dotted column name appears specifically inside the `Maybe you meant: [...]` suggestion list rather than anywhere in the message, so re-introducing the filter (which would empty the suggestion list) is caught directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion list The test exercises the line PR #1894 removes (the dot-filter in `ColumnsDescription::getAllRegisteredNames`). It now asserts the dotted column name appears specifically inside the `Maybe you meant: [...]` suggestion list rather than anywhere in the message, so re-introducing the filter (which would empty the suggestion list) is caught directly.
…Memory-engine 04259 `04261_iceberg_dotted_array.sh` reads dotted-name `Array` columns through the actual Iceberg read path (`IcebergLocal` engine + `icebergLocal` table function with an explicitly-declared schema), which the previous tests did not cover: * a lone `` `a.b` Array(String) `` column must read back its stored values (the core regression for ClickHouse#90731 — it previously came back empty), and * a lone dotted column alongside dotted columns that share a prefix (`` `c.x` ``, `` `c.y` ``) — all must round-trip correctly. `04259_dotted_array_not_nested.sql` is removed: it used `ENGINE = Memory`, which does not exercise the Iceberg read path (flagged in review) and does not go through `Nested::convertToSubcolumns` / `collect`, so it neither reproduced Iceberg test above, while the hint-path change is covered by `04260_dotted_column_in_hints.sh`.
6655b2b to
def81c2
Compare
|
Correct me if I'm wrong but from what I see none of the stateless tests can actually test that this issue has been fixed. The reason for that is that in the original issue the data was written by Spark. So, this can be verified with an integration test. Your integration test indeed does check the issue. But there's another problem with that test: it passes on I tried to investigate that with Cursor, and Cursor states that commits 0a218cd and 4b733ba (which are already in |
|
From what I can see this PR at this point only effectively changes the error message from to This is a nice to have feature, but it's not what the original issue was about.
|
Actually there are some stateless tests that use pre-generated iceberg tables. (E.g. see But here this is not the case. |
|
@zvonand @mkmkme The finding
Why I went the long way around (and missed the simple truth).
The thing that should have caught this on day one maybe the quick review/verification I skipped, which is does the repro pass on base, without my change? I only added an Iceberg test ( Dropping it; the real coverage is the Spark integration test that shipped with 0a218cd/4b733ba. Lesson for next time: before trusting a regression test, run it on base first — a test that passes without the change proves the change does nothing. I should have done that before writing a line of fix code. Closing as superseded by 0a218cd/4b733ba. Thanks for the careful review; it saved us shipping a no-op. |

Backport of upstream ClickHouse/ClickHouse#105546 (commit
f8467af) ontoantalya-26.3. Re-opened fromAltinity/ClickHouse(instead of a fork) so CI publishes direct.debpackage URLs for clickhouse-regression. Cherry-pick applied cleanly with no contextual conflicts.Upstream issue: ClickHouse/ClickHouse#90731
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix reading Iceberg tables whose
ARRAYcolumn names contain a dot (e.g.`a.b` ARRAY<STRING>), which previously returned empty arrays. Two upstream defects were responsible:ColumnsDescription::getAllRegisteredNamesfiltered out dotted names, andNestedUtils::getSubcolumnsOfNestedmisclassified lone dottedArray(T)columns as flattenedNestedchildren.Documentation entry for user-facing changes
Symptom
When querying an Iceberg table through the
iceberg(...)table function or aDataLakeCatalog, a column whose name contains a.and whose type isArray(T)returned empty arrays instead of the stored values. The same data read by Spark returned the expected values.Root cause
The Parquet V3 reader path (
SchemaConverter+ColumnMapper+FormatFilterInfo) is already correct after the dotted-name field-id work in0a218cd4e8b,4b733bae561andf24c1a46063(present inantalya-26.3). The remaining symptom comes from two upstream defects, independent of Iceberg but exposed by it:ColumnsDescription::getAllRegisteredNamesexplicitly filtered out any column whose name contained., assuming such names were always flattened Nested subcolumns. A column whose stored name literally contains a dot (allowed by MergeTree with backticks, produced by Iceberg/Spark) is a first-class registered name. The function is only consumed byIHints-style suggestion paths (andStorageSystemZooKeepercolumn-name iteration, where no dotted names exist), so relaxing it has no effect on parsing, planning, storage, or wire protocol.NestedUtils::getSubcolumnsOfNestedtreated every dotted-nameArray(T)column as a flattened element of a syntheticNestedstructure named after the prefix. This made the Arrow, ORC and pre-V3 Parquet readers look for a struct field with the prefix name rather than the literal dotted column, returning an empty array.Fix
ColumnsDescription::getAllRegisteredNames— drop the dot filter; return every registered column name.NestedUtils::getSubcolumnsOfNested— two-pass scan: a syntheticNestedentry is emitted only when at least twoArray(T)columns share the same dotted prefix. A lonea.b: Array(T)no longer appears in the synthetic-Nested map. Genuine flattenedNestedwith multiple fields is unaffected; the existing early-continue onisNested()covers the one-field edge case.Tests
tests/integration/test_storage_iceberg_with_spark/test_column_names_with_dots.py::test_dotted_array_column— end-to-end repro of Iceberg: Selecting from ARRAY column with dot-separated name returns empty lists ClickHouse/ClickHouse#90731 againsts3,azureandlocalstorage.…::test_dotted_array_alongside_real_nested— mixed-schema guard: a lone dottedArraycolumn coexists with a genuine flattened-Nested sibling group.tests/queries/0_stateless/04259_dotted_array_not_nested.sql— isolates theNestedUtilsfix without Iceberg (Memory engine).tests/queries/0_stateless/04260_dotted_column_in_hints.sh— verifies theColumnsDescriptionfix via misspelling-hint output.Risk / scope
Low. Five-line removal in
ColumnsDescription(hint suggestions only) and a localised two-pass refactor inNestedUtils::getSubcolumnsOfNested. No header changes, no new settings, no public API change. Parquet V3 path is not modified.CI/CD Options
Exclude tests:
Regression jobs to run: