Skip to content

Fix typo in complex schema evolution - #111489

Merged
scanhex12 merged 1 commit into
masterfrom
iceberg_complex_schema_evolution_fix
Jul 23, 2026
Merged

Fix typo in complex schema evolution#111489
scanhex12 merged 1 commit into
masterfrom
iceberg_complex_schema_evolution_fix

Conversation

@scanhex12

@scanhex12 scanhex12 commented Jul 22, 2026

Copy link
Copy Markdown
Member

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 typo in complex schema evolution

Version info

  • Merged into: 26.7.1.1378 (included in 26.7 and later)
  • Backported to: 26.7.2.48, 26.6.2.105, 26.5.6.92, 26.4.5.167, 26.3.17.93

@clickhouse-gh

clickhouse-gh Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [9bae326]

Summary:

job_name test_name status info comment
Fast test (arm_darwin) FAIL
Server died FAIL cidb
Stress test (arm_msan) FAIL
Cannot start clickhouse-server FAIL cidb
Check failed FAIL cidb

AI Review

Summary

This PR fixes one bad variant assignment in Iceberg complex schema evolution and adds a regression test for nested Array(Map(Struct(...))) columns. That immediate STRUCT case looks covered, but the same map-value traversal contract is still broken in traverseAllPaths for deeper descendants below the map value, so I do not think the fix is complete yet.

PR Metadata
  • Changelog category: Bug Fix matches the user-visible Iceberg schema-evolution regression.
  • Changelog entry: required, but Fix typo in complex schema evolution is too internal and vague for the changelog. Suggested replacement: Fixes Iceberg complex schema evolution for nested \Array(Map(Struct(...)))` columns so `ADD COLUMN`, `ALTER COLUMN`, `RENAME COLUMN`, and `DROP COLUMN` no longer throw an exception.`
Findings
  • ⚠️ Major: [src/Storages/ObjectStorage/DataLakes/Iceberg/ComplexTypeSchemaProcessorFunctions.cpp:171-190] The map-value traversal contract is still incomplete. transform now carries MAP children correctly, but traverseAllPaths still enqueues the whole (key, value) entry instead of the map value, so paths like x.element.value.element... or x.element.value.value... reach the next ARRAY or MAP step with a Tuple and still throw an exception. Suggested fix: read next_val.safeGet<Tuple>()[1] in that MAP branch as well, then add a regression that nests an ARRAY or MAP under the map value.
Tests
  • ⚠️ The new integration test only covers map values that are already STRUCTs, which means traverseAllPaths never has to descend past the map entry. A focused case like Array(Map(Int, Array(Struct(...)))) or Map(Int, Array(Struct(...))) would prove the deeper map-value traversal is wired correctly.
Final Verdict

Status: ⚠️ Request changes

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 22, 2026
@scanhex12 scanhex12 added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Jul 22, 2026
else if (current_tuple[subfield_index].tryGet(tmp_node_map))
{
current_node = std::move(tmp_node_array);
current_node = std::move(tmp_node_map);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the ARRAY branch here is necessary, but it still leaves deeper descendants under a map value broken. In traverseAllPaths (ComplexTypeSchemaProcessorFunctions.cpp:171-190) the MAP case pushes next_val itself, i.e. the whole (key, value) tuple, instead of next_val.safeGet<Tuple>()[1]; a path such as x.element.value.element... or x.element.value.value... then reaches the next ARRAY/MAP step with a Tuple and still throws before transform runs. Please mirror the MAP handling there as well and add a regression where the map value is an ARRAY or MAP, not just a STRUCT.

@clickhouse-gh

clickhouse-gh Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

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

Changed lines: Changed C/C++ lines covered: 0/2 (0.00%) · Uncovered code

Full report · Diff report

@scanhex12
scanhex12 enabled auto-merge July 23, 2026 06:56
@scanhex12
scanhex12 added this pull request to the merge queue Jul 23, 2026
Merged via the queue into master with commit d8fb1c3 Jul 23, 2026
176 of 179 checks passed
@scanhex12
scanhex12 deleted the iceberg_complex_schema_evolution_fix branch July 23, 2026 09:58
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 23, 2026
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 23, 2026
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jul 23, 2026
clickhouse-gh Bot added a commit that referenced this pull request Jul 23, 2026
Backport #111489 to 26.6: Fix typo in complex schema evolution
alexey-milovidov added a commit that referenced this pull request Jul 24, 2026
Backport #111489 to 26.4: Fix typo in complex schema evolution
scanhex12 added a commit that referenced this pull request Jul 31, 2026
Backport #111489 to 26.5: Fix typo in complex schema evolution
scanhex12 added a commit that referenced this pull request Jul 31, 2026
Backport #111489 to 26.3: Fix typo in complex schema evolution
scanhex12 added a commit that referenced this pull request Jul 31, 2026
Backport #111489 to 26.7: Fix typo in complex schema evolution
@scanhex12 scanhex12 added v26.2-must-backport and removed pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Aug 3, 2026
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Aug 3, 2026
@robot-clickhouse robot-clickhouse added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo v26.2-must-backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants