Skip to content

Fix hardlink with indexes and projections#107753

Draft
scanhex12 wants to merge 2 commits into
masterfrom
scanhex12-patch-1
Draft

Fix hardlink with indexes and projections#107753
scanhex12 wants to merge 2 commits into
masterfrom
scanhex12-patch-1

Conversation

@scanhex12

@scanhex12 scanhex12 commented Jun 17, 2026

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@scanhex12 scanhex12 marked this pull request as draft June 17, 2026 15:08
@clickhouse-gh

clickhouse-gh Bot commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [2b14a50]

Summary: ❌

job_name test_name status info comment
Stress test (amd_msan) FAIL
Hung check failed, possible deadlock found FAIL cidb

AI Review

Summary

This PR changes MutateAllPartColumnsTask to classify the mutation source by ctx->source_part storage when deciding whether existing skip indexes and projections can be hardlinked or must be rebuilt. The direction looks consistent with the surrounding splitAndModifyMutationCommands, updateIndicesToRecalculateAndDrop, and projection-recalculation helpers, but the PR does not include evidence for the non-full storage path it changes.

Missing context / blind spots
  • ⚠️ The PR body does not describe the failing scenario, and the OSS checkout does not instantiate a non-full IDataPartStorage implementation that can locally exercise the changed branch. A linked failing CI report or a focused private/Cloud regression test would close this gap.
  • ⚠️ PR CI was still pending at review time; Fast test and Build (arm_tidy) had not completed, and CI Tests / bugfix validation jobs were skipped.
Tests
  • ⚠️ [src/Storages/MergeTree/MutateTask.cpp:2030] Please add or link a focused regression test for a non-full source part with existing skip indexes and a projection where mutation must rebuild them instead of hardlinking. Existing OSS tests only cover DataPartStorageOnDiskFull, so they would not fail if this line regressed back to ctx->new_data_part.
Final Verdict

Status: ⚠️ Request changes

Minimum required action: add or link focused regression coverage for the non-full storage hardlink/rebuild behavior changed here.

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 17, 2026
clickhouse-gh[bot]
clickhouse-gh Bot reviewed Jun 17, 2026
View reviewed changes
Comment thread src/Storages/MergeTree/MutateTask.cpp
NameSet removed_projections;

bool is_full_part_storage = isFullPartStorage(ctx->new_data_part->getDataPartStorage());
bool is_full_part_storage = isFullPartStorage(ctx->source_part->getDataPartStorage());

@clickhouse-gh clickhouse-gh Bot Jun 17, 2026

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.

This fixes the storage-type discriminator used for mutation hardlink/rebuild decisions, but I do not see a regression test that would fail if this still looked at ctx->new_data_part. Existing OSS tests instantiate DataPartStorageOnDiskFull, so they do not cover the !isFullPartStorage(ctx->source_part->getDataPartStorage()) branch this patch changes. Please add or link a focused test for a non-full source part with existing skip indexes and a projection where the mutation must rebuild them instead of hardlinking them.

@arsenmuk arsenmuk self-assigned this Jun 17, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.20% 85.30% +0.10%
Functions 92.30% 92.30% +0.00%
Branches 77.50% 77.50% +0.00%

Changed lines: Changed C/C++ lines covered by tests: 4/4 (100.00%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 1 line(s) · 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

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

Assignees

@arsenmuk arsenmuk

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.

2 participants

@scanhex12 @arsenmuk