Fix `assertTypeEquality` to check both sides for wrapper column types by alexey-milovidov · Pull Request #101105 · ClickHouse/ClickHouse · GitHub
Skip to content

Fix assertTypeEquality to check both sides for wrapper column types#101105

Merged
alexey-milovidov merged 20 commits into
masterfrom
fix/assertTypeEquality-check-both-sides
May 1, 2026
Merged

Fix assertTypeEquality to check both sides for wrapper column types#101105
alexey-milovidov merged 20 commits into
masterfrom
fix/assertTypeEquality-check-both-sides

Conversation

@alexey-milovidov

@alexey-milovidov alexey-milovidov commented Mar 29, 2026

Copy link
Copy Markdown
Member

The assertion in IColumn::assertTypeEquality only checked if this was a Const/Sparse/Replicated wrapper column, but not rhs. When rhs is a wrapper column but this is a regular column, the assertion incorrectly required typeid equality, which fails even though the operation is valid.

This caused a spurious logical error exception in sanitizer builds during stress tests (STID: 2508-328f).

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Version info

  • Merged into: 26.5.1.214

The assertion in `IColumn::assertTypeEquality` only checked if `this`
was a Const/Sparse/Replicated wrapper column, but not `rhs`. When `rhs`
is a wrapper column but `this` is a regular column, the assertion
incorrectly required `typeid` equality, which fails even though the
operation is valid.

This caused a spurious logical error exception in sanitizer builds
during stress tests (STID: 2508-328f).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alexey-milovidov

Copy link
Copy Markdown
Member Author

@clickhouse-gh

clickhouse-gh Bot commented Mar 29, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [010a8bb]

Summary:


AI Review

Summary

This PR fixes IColumn::assertTypeEquality so wrapper-column compatibility checks apply when either side is a wrapper (Const, Sparse, or Replicated), and adds a regression test for the ColumnReplicated-on-rhs path hit by merge after RIGHT JOIN. I did not find correctness, safety, performance, or compatibility issues in the change set.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
No large/binary files
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-ci label Mar 29, 2026
alexey-milovidov and others added 6 commits March 29, 2026 21:34
Tests that `IColumn::assertTypeEquality` correctly handles the case where
`rhs` is a `ColumnReplicated` wrapper but `this` is a regular column.
This triggers `MergingSortedAlgorithm::merge` with `ColumnReplicated`
columns produced by RIGHT JOIN with `enable_lazy_columns_replication`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alexey-milovidov

Copy link
Copy Markdown
Member Author

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

alexey-milovidov and others added 13 commits April 6, 2026 17:49
…itions

The test was originally added as `04066_assert_type_equality_column_replicated`,
but master has since added three other tests with the same `04066` prefix.
Rename to the next available prefix `04106`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.10% +0.10%
Functions 91.10% 91.10% +0.00%
Branches 76.50% 76.60% +0.10%

Changed lines: N/A (no coverable changed lines) · Uncovered code

Full report · Diff report

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good.

@alexey-milovidov alexey-milovidov self-assigned this May 1, 2026
@alexey-milovidov alexey-milovidov added this pull request to the merge queue May 1, 2026
Merged via the queue into master with commit 1ce0c3b May 1, 2026
165 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-ci 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.

2 participants