Skip to content

validate column structure before applying patches#99531

Merged
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
seva-potapov:fix-patch-parts-type-mismatch-sigsegv
Mar 20, 2026
Merged

validate column structure before applying patches#99531
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
seva-potapov:fix-patch-parts-type-mismatch-sigsegv

Conversation

@seva-potapov
Copy link
Copy Markdown
Contributor

@seva-potapov seva-potapov commented Mar 16, 2026

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):

validate column structure before applying patches

Details:

applyPatchesToBlockRaw matches patch columns to result columns by name only, without type validation. When schema evolution changes a column type (e.g., UInt32 change to String), the patch column has a different type than the result column that results in crash.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 16, 2026

Workflow [PR], commit [2c2c2d3]

Summary:


AI Review

Summary

This PR hardens patch application across schema evolution by validating per-column types before applying updates, adds per-source filtering in the combined patch path, and adds regression coverage for raw and mixed-source scenarios. I did not find additional high-confidence correctness/safety/performance issues in the current PR head beyond already-posted inline discussion.

Missing context

  • ⚠️ No CI run/log context was provided in this request, so this review is code-only.

ClickHouse Rules

Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsHistory.cpp
Safe rollout
Compilation time

Final Verdict

  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 16, 2026
Comment thread src/Storages/MergeTree/PatchParts/applyPatches.cpp Outdated
@seva-potapov seva-potapov force-pushed the fix-patch-parts-type-mismatch-sigsegv branch from 5cc4acb to 2ac57aa Compare March 16, 2026 01:39
Comment thread src/Storages/MergeTree/PatchParts/applyPatches.cpp
Comment thread src/Storages/MergeTree/PatchParts/applyPatches.cpp Outdated
@seva-potapov seva-potapov force-pushed the fix-patch-parts-type-mismatch-sigsegv branch from dfcfd2b to 2c2c2d3 Compare March 16, 2026 03:40
@seva-potapov seva-potapov requested a review from CurtizJ March 16, 2026 09:33
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 16, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.70% 83.70% +0.00%
Functions 23.90% 23.90% +0.00%
Branches 76.20% 76.20% +0.00%

PR changed lines: PR changed-lines coverage: 98.41% (186/189, 1 noise lines excluded)
Diff coverage report
Uncovered code

@alexey-milovidov
Copy link
Copy Markdown
Member

Is it possible to have an end-to-end .sql/.sh test in addition to a unit test?

@seva-potapov
Copy link
Copy Markdown
Contributor Author

I have looked into it, Alexey, but to replicate the issue we've had on the cloud, we need SMT and shared catalog in order to arrive at schema evolution outside of ALTER MODIFY (which doesn't have the problem we are fixing here).

@alexey-milovidov alexey-milovidov added this pull request to the merge queue Mar 20, 2026
@alexey-milovidov alexey-milovidov self-assigned this Mar 20, 2026
Merged via the queue into ClickHouse:master with commit 85e1445 Mar 20, 2026
321 of 322 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 20, 2026
@evillique
Copy link
Copy Markdown
Member

I think this fix is incorrect; we silently lose the data from LWU that was in progress if we ALTER the column type.

This fix looks more correct to me: #100107

@seva-potapov
Copy link
Copy Markdown
Contributor Author

thank you, @evillique ! your fix is definitely more correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants