Skip to content

Fix crash in Iceberg due to type confusion#109551

Merged
mstetsyuk merged 2 commits into
masterfrom
fix-iceberg-equality-delete-nullability
Jul 8, 2026
Merged

Fix crash in Iceberg due to type confusion#109551
mstetsyuk merged 2 commits into
masterfrom
fix-iceberg-equality-delete-nullability

Conversation

@mstetsyuk

@mstetsyuk mstetsyuk commented Jul 6, 2026

Copy link
Copy Markdown
Member

Linked issues

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC)

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

Fix a crash when reading Iceberg tables with equality delete files. If a column is nullable in the equality delete file but non-nullable in the table schema (or vice versa), the values read from the delete file were inserted into a column of a different type through an unchecked cast (a column type confusion), corrupting the column and crashing the server.

Version info

  • Merged into: 26.7.1.664 (included in 26.7 and later)
  • Backported to: 26.6.2.50

@mstetsyuk
mstetsyuk requested a review from hanfei1991 July 6, 2026 17:11
@clickhouse-gh

clickhouse-gh Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [8556ac7]

Summary:


AI Review

Summary

This PR fixes the immediate ColumnNullable/non-ColumnNullable type confusion when building equality-delete sets by taking the key type from the delete-file header instead of the table schema. That addresses the unchecked cast which can corrupt the set column, but the nullable equality-delete path is still semantically wrong: the filter is built with plain notIn, so nullable table rows with NULL in the equality key are still filtered out even when the delete file contains no NULL key. Final verdict: changes requested.

Findings

⚠️ Majors

  • [src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp:1251] Using the delete-file header type fixes the insert-side type confusion, but the downstream predicate still uses plain notIn on nullable equality keys. In ClickHouse, NULL NOT IN (...) yields NULL, and [src/Columns/FilterDescription.cpp:111] converts nullable filter results to false, so any data row whose equality key is NULL is dropped whenever an equality-delete filter is present, even if the delete set contains no NULL. That violates the PR’s promised required delete file -> optional table fix and needs a null-safe predicate (notNullIn or equivalent) plus a regression that preserves NULL rows when the delete file only contains non-NULL keys.
Final Verdict

Changes requested.

@clickhouse-gh clickhouse-gh Bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! labels Jul 6, 2026
@cho11y

cho11y Bot commented Jul 6, 2026

Copy link
Copy Markdown

Backport labels have been automatically added based on https://github.com/clickhouse/clickhouse-core-incidents/issues/1882

Labels added:

  • v26.2-must-backport

These labels ensure backport PRs are properly created for each affected release version. The release automation will create backport PRs as needed based on these labels.

@alexey-milovidov

Copy link
Copy Markdown
Member

Updating the branch to fix the 04001_join_reorder_through_expression test, which was broken today in master.

/// Take the type from the delete file header rather than from the table schema:
/// the nullability of a column in the delete file may differ from its nullability
/// in the table schema, and the columns read below have exactly the header's types.
block_for_set.insert(ColumnWithTypeAndName(delete_file_header.getByPosition(position_in_delete_file).type, name_and_type.name));

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.

Using the delete-file header type fixes the insertRangeFrom type confusion, but it still leaves nullable equality keys semantically wrong downstream. This path later builds the predicate with plain notIn (see IcebergMetadata.cpp:1313), and for a nullable left-hand key ClickHouse returns NULL rather than 1/0 when the row value itself is NULL (for example SELECT NULL NOT IN (1) is NULL). FilterDescription then turns that nullable filter result into false, so every data row whose equality key is NULL gets dropped whenever an equality-delete filter is present, even if the delete set contains no NULL at all.

That means the reverse direction claimed in the PR body (required delete file -> optional table) is still incorrect today for real nullable rows. This probably needs a null-safe predicate here (notNullIn or an explicit null-aware comparison) plus a regression that keeps NULL rows when the delete file only contains non-NULL keys.

@clickhouse-gh

clickhouse-gh Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.60% 85.60% +0.00%
Functions 92.70% 92.70% +0.00%
Branches 77.80% 77.80% +0.00%

Changed lines: Changed C/C++ lines covered: 6/6 (100.00%) · Uncovered code

Full report · Diff report

@hanfei1991 hanfei1991 self-assigned this Jul 6, 2026
@PedroTadim

Copy link
Copy Markdown
Member

So I see Spark doesn't generate equality deletes. Let me add this coverage in #109194

@clickhouse-gh

clickhouse-gh Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 37 queries analysed

no significant changes detected. K_source=6 K_base=30 flagged=0/65

clickbench

🟢 No significant changes

tpch_adapted_1_official

🟢 No significant changes

Debug info
  • StressHouse run: 9618ca83-e4a8-4512-9724-939a9ee0d696
  • MIRAI run: f7e4cb1a-5890-4706-a070-fb393af9f3a8
  • PR check IDs:
    • clickbench_9229_1783511919
    • clickbench_9237_1783511919
    • clickbench_9246_1783511919
    • tpch_adapted_1_official_9255_1783511919
    • tpch_adapted_1_official_9269_1783511919
    • tpch_adapted_1_official_9281_1783511919

@mstetsyuk
mstetsyuk enabled auto-merge July 8, 2026 13:17
@mstetsyuk
mstetsyuk added this pull request to the merge queue Jul 8, 2026
Merged via the queue into master with commit fd59515 Jul 8, 2026
175 checks passed
@mstetsyuk
mstetsyuk deleted the fix-iceberg-equality-delete-nullability branch July 8, 2026 13:33
@robot-clickhouse robot-clickhouse added 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 labels Jul 8, 2026
clickhouse-gh Bot added a commit that referenced this pull request Jul 8, 2026
Backport #109551 to 26.6: Fix crash in Iceberg due to type confusion
@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 18, 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-critical-bugfix 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.

6 participants