Skip to content

Fix shared variant column pointers in ColumnVariant::filter#100234

Merged
alexey-milovidov merged 5 commits intomasterfrom
fix-variant-bad-ptrs
Mar 22, 2026
Merged

Fix shared variant column pointers in ColumnVariant::filter#100234
alexey-milovidov merged 5 commits intomasterfrom
fix-variant-bad-ptrs

Conversation

@Avogar
Copy link
Copy Markdown
Member

@Avogar Avogar commented Mar 20, 2026 •
edited
Loading

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

Fixed a LOGICAL_ERROR exception in queries involving Dynamic columns with cross joins and runtime filters, caused by ColumnVariant::filter sharing variant column pointers instead of cloning them in the hasOnlyNulls optimization path. Closes #100147

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@Avogar
Fix ColumnVariant::filter sharing variant column pointers in hasOnlyN…
0a4e9a2 
…ulls path and replace unsafe WrappedPtr with MutableColumnPtr in Set::set_elements.
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 20, 2026 •
edited
Loading

Workflow [PR], commit [1d7fec1]

Summary: ✅

@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 Mar 20, 2026
@alexey-milovidov alexey-milovidov self-assigned this Mar 20, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 21, 2026

AI Review

Summary

This PR fixes a real correctness issue in ColumnVariant::filter by avoiding shared variant-column pointers in the hasOnlyNulls path and adds regression coverage. However, one blocker remains in tests: a new .sql file was added with an invisible Unicode character in its filename, which can break test discovery/pairing. Also, PR metadata should use a non-critical changelog category because the issue is a LOGICAL_ERROR exception, not a crash/data loss/RBAC incident.

PR Metadata

  • Changelog category is semantically mismatched: Critical Bug Fix (crash, data loss, RBAC) does not match this change.
  • Changelog entry is present and specific enough.
  • Suggested replacement category:
    • Bug Fix

Findings

❌ Blockers

  • [tests/queries/0_stateless/04042_hash_join_allocated_size_tracking.sql (contains U+200E in filename):1] The filename contains an invisible U+200E character. This can make test discovery and *.sql/*.reference pairing inconsistent, causing CI instability.
    • Suggested fix: rename the file to tests/queries/0_stateless/04042_hash_join_allocated_size_tracking.sql (no hidden characters).

💡 Nits

  • [PR template metadata] Changelog category should be Bug Fix instead of Critical Bug Fix for an exception fix.

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 ⚠️ Critical Bug Fix does not match this change; use Bug Fix.
Safe rollout ✅
Compilation time ✅

Final Verdict

  • Status: ⚠️ Request changes
  • Minimum required actions:
    • Rename tests/queries/0_stateless/04042_hash_join_allocated_size_tracking.sql to remove the invisible U+200E character.
    • Update PR Changelog category to Bug Fix.

alexey-milovidov and others added 2 commits March 21, 2026 11:15
@alexey-milovidov @claude
Remove invisible Unicode character (U+200E) from test filename
7f179fc 
The file `04042_hash_join_allocated_size_tracking.sql` had a hidden
LEFT-TO-RIGHT MARK character appended to its name, which could cause
test discovery tooling to skip it.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@alexey-milovidov
Update 04042_hash_join_allocated_size_tracking.sql
1d7fec1
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 21, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.80% 83.80% +0.00%
Functions 24.40% 24.50% +0.10%
Branches 76.40% 76.40% +0.00%

PR changed lines: PR changed-lines coverage: 96.15% (25/26, 1 noise lines excluded)
Diff coverage report
Uncovered code

@alexey-milovidov alexey-milovidov merged commit e12cd5d into master Mar 22, 2026
151 of 152 checks passed
@alexey-milovidov alexey-milovidov deleted the fix-variant-bad-ptrs branch March 22, 2026 10:16
@robot-ch-test-poll1 robot-ch-test-poll1 mentioned this pull request Mar 22, 2026
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Mar 22, 2026
@robot-ch-test-poll1
Merge pull request #100335 from ClickHouse/cherrypick/25.3/100234
299b757 
Cherry pick #100234 to 25.3: Fix shared variant column pointers in `ColumnVariant::filter`
robot-clickhouse added a commit that referenced this pull request Mar 22, 2026
@robot-clickhouse
Backport #100234 to 25.3: Fix shared variant column pointers in `Colu…
067edc1 
…mnVariant::filter`
This was referenced Mar 22, 2026
Closed
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Mar 22, 2026
@robot-ch-test-poll1
Merge pull request #100337 from ClickHouse/cherrypick/25.8/100234
a46d550 
Cherry pick #100234 to 25.8: Fix shared variant column pointers in `ColumnVariant::filter`
robot-clickhouse added a commit that referenced this pull request Mar 22, 2026
@robot-clickhouse
Backport #100234 to 25.8: Fix shared variant column pointers in `Colu…
2141781 
…mnVariant::filter`
This was referenced Mar 22, 2026
Merged
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Mar 22, 2026
@robot-ch-test-poll1
Merge pull request #100339 from ClickHouse/cherrypick/25.12/100234
c8eeca9 
Cherry pick #100234 to 25.12: Fix shared variant column pointers in `ColumnVariant::filter`
robot-clickhouse added a commit that referenced this pull request Mar 22, 2026
@robot-clickhouse
Backport #100234 to 25.12: Fix shared variant column pointers in `Col…
eed04ae 
…umnVariant::filter`
This was referenced Mar 22, 2026
Closed
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Mar 22, 2026
@robot-ch-test-poll1
Merge pull request #100341 from ClickHouse/cherrypick/26.1/100234
77df1eb 
Cherry pick #100234 to 26.1: Fix shared variant column pointers in `ColumnVariant::filter`
robot-clickhouse added a commit that referenced this pull request Mar 22, 2026
@robot-clickhouse
Backport #100234 to 26.1: Fix shared variant column pointers in `Colu…
c13d13a 
…mnVariant::filter`
This was referenced Mar 22, 2026
Merged
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Mar 22, 2026
@robot-ch-test-poll1
Merge pull request #100343 from ClickHouse/cherrypick/26.2/100234
8ab674d 
Cherry pick #100234 to 26.2: Fix shared variant column pointers in `ColumnVariant::filter`
robot-clickhouse added a commit that referenced this pull request Mar 22, 2026
@robot-clickhouse
Backport #100234 to 26.2: Fix shared variant column pointers in `Colu…
fb9e469 
…mnVariant::filter`
This was referenced Mar 22, 2026
Merged
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Mar 22, 2026
@robot-ch-test-poll1
Merge pull request #100345 from ClickHouse/cherrypick/26.3/100234
e3b0958 
Cherry pick #100234 to 26.3: Fix shared variant column pointers in `ColumnVariant::filter`
robot-clickhouse added a commit that referenced this pull request Mar 22, 2026
@robot-clickhouse
Backport #100234 to 26.3: Fix shared variant column pointers in `Colu…
da76ef0 
…mnVariant::filter`
@robot-ch-test-poll1 robot-ch-test-poll1 mentioned this pull request Mar 22, 2026
Merged
@robot-clickhouse robot-clickhouse added pr-synced-to-cloud The PR is synced to the cloud repo pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Mar 22, 2026
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Mar 22, 2026
clickhouse-gh bot added a commit that referenced this pull request Mar 22, 2026
@clickhouse-gh
Merge pull request #100344 from ClickHouse/backport/26.2/100234
848c242 
Backport #100234 to 26.2: Fix shared variant column pointers in `ColumnVariant::filter`
clickhouse-gh bot added a commit that referenced this pull request Mar 22, 2026
@clickhouse-gh
Merge pull request #100342 from ClickHouse/backport/26.1/100234
74cdc4a 
Backport #100234 to 26.1: Fix shared variant column pointers in `ColumnVariant::filter`
@nickitat nickitat linked an issue Mar 23, 2026 that may be closed by this pull request
Logical error: data->allocated_size != debug_allocated_size (A != B) #99920
Closed
alexey-milovidov added a commit that referenced this pull request Mar 23, 2026
@alexey-milovidov
Merge pull request #100346 from ClickHouse/backport/26.3/100234
62c65b2 
Backport #100234 to 26.3: Fix shared variant column pointers in `ColumnVariant::filter`
@Avogar Avogar mentioned this pull request Mar 24, 2026
Closed
nikitamikhaylov added a commit that referenced this pull request Mar 31, 2026
@nikitamikhaylov
Merge pull request #100338 from ClickHouse/backport/25.8/100234
44d85e4 
Backport #100234 to 25.8: Fix shared variant column pointers in `ColumnVariant::filter`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clickhouse-gh clickhouse-gh[bot] clickhouse-gh[bot] left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Logical error: data->allocated_size != debug_allocated_size (A != B)

4 participants

@Avogar @alexey-milovidov @robot-clickhouse @robot-ch-test-poll3