Skip to content

Native Common Virtuals for FileLog#101006

Open
Michicosun wants to merge 2 commits intomasterfrom
native_common_virtuals/FileLog
Open

Native Common Virtuals for FileLog#101006
Michicosun wants to merge 2 commits intomasterfrom
native_common_virtuals/FileLog

Conversation

@Michicosun
Copy link
Copy Markdown
Member

@Michicosun Michicosun commented Mar 28, 2026

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Part of #100766

@clickhouse-gh
Copy link
Copy Markdown
Contributor

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

Workflow [PR], commit [7f2f879]

Summary: ❌

job_name test_name status info comment
Stateless tests (arm_asan_ubsan, targeted) failure
04031_filelog_drop_mv_no_exception FAIL cidb
Stateless tests (amd_debug, parallel) failure
03668_shard_join_in_reverse_order FAIL cidb, issue
Stateless tests (amd_tsan, sequential, 1/2) failure
Server died FAIL cidb
01007_r1r2_w_r2r1_deadlock UNKNOWN cidb
01004_rename_deadlock UNKNOWN cidb
ThreadSanitizer: data race (STID: 3939-3dff) FAIL cidb
Scraping system tables FAIL cidb
Stateless tests (amd_tsan, sequential, 2/2) failure
ThreadSanitizer: data race (STID: 5042-4e5a) FAIL cidb

AI Review

Summary

This PR makes common virtual columns storage-specific and updates all lookups to use storage.getCommonVirtuals(). It also makes FileLog expose _table as a native storage virtual column and fills it explicitly in FileLogSource. I did not find correctness, safety, concurrency, or performance issues in the diff. High-level verdict: the change is coherent and safe.

Missing context
  • ⚠️ No CI results/logs were provided in the review context, so runtime/test verification could not be cross-checked here.
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 ✅
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 28, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 28, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.20% 84.10% -0.10%
Functions 24.50% 24.50% +0.00%
Branches 76.70% 76.70% +0.00%

Changed lines: 96.08% (98/102) · Uncovered code

Full report · Diff report

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

Reviewers

No reviews

Assignees

No one assigned

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Michicosun