ClickHouse / ClickHouse/ClickHouse

createLightweightDeleteCommand does not copy mutation_version from source command

Open
#101,874 3 comments 0 reactions 1 assignee Claimed by @CurtizJ View on GitHub
bug comp-mergetree comp-mutations
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

_Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment._

_Retrospective finding from a historical scan of [PR #79281](https://github.com/ClickHouse/ClickHouse/pull/79281) (merged 2025-05-13). Confirmed on current codebase — close with a note if already fixed._

### Describe what's wrong

When a lightweight delete (UPDATE _row_exists=0) is converted to a DELETE command by createLightweightDeleteCommand, the mutation_version is lost (defaults to nullopt). This causes perform_alter_conversions to be incorrectly computed as false when combined with ALTER MODIFY COLUMN, because nullopt > version_of_alter_mutation always returns false.

**Root cause:** AlterConversions.cpp:102 - MutationCommand::parse(*alter_command) does not set mutation_version (it's not stored in ASTAlterCommand). The function should copy command.mutation_version to the result, like createCommandWithUpdatedColumns does at line 59.

**Why we believe this is a bug:** AlterConversions.cpp:90-108 createLightweightDeleteCommand() → MutationCommand::parse() → returns command with mutation_version=nullopt. Compare with createCommandWithUpdatedColumns() at line 59 which correctly copies mutation_version. At line 305, perform_alter_conversions = !version_of_alter_mutation || actions.mutation_version > version_of_alter_mutation evaluates incorrectly when mutation_version is nullopt.

**Affected locations:**
- `src/Storages/MergeTree/AlterConversions.cpp:102` — createLightweightDeleteCommand: mutation_version not copied
- `src/Storages/MergeTree/AlterConversions.cpp:305` — perform_alter_conversions uses mutation_version for comparison

**Impact:** When lightweight DELETE + ALTER MODIFY COLUMN are both pending with apply_mutations_on_fly, the DELETE step will always have perform_alter_conversions=false regardless of mutation ordering, causing LOGICAL_ERROR crash (Bad cast between column types). Currently masked by a broader pre-existing issue where ANY DELETE + ALTER MODIFY + on-fly crashes.

### Does it reproduce on most recent release?

Yes — confirmed on current `master` (commit `11ce418c0505`).

### How to reproduce

```sql
-- Test: lightweight DELETE + ALTER MODIFY COLUMN with apply_mutations_on_fly
-- Covers: AlterConversions.cpp - DELETE mutation combined with ALTER MODIFY COLUMN on fly

DROP TABLE IF EXISTS t_lwd_modify_fly;
CREATE TABLE t_lwd_modify_fly (a UInt32, b Int32) ENGINE = MergeTree ORDER BY a;
INSERT INTO t_lwd_modify_fly VALUES (1, 10) (2, 20) (3, 30);

-- Async lightweight delete
DELETE FROM t_lwd_modify_fly WHERE a = 1 SETTINGS lightweight_deletes_sync = 0;
-- Async ALTER MODIFY COLUMN (type change)
ALTER TABLE t_lwd_modify_fly MODIFY COLUMN b Int64 SETTINGS mutations_sync = 0, alter_sync = 0;

SYSTEM STOP MERGES t_lwd_modify_fly;

-- This should return rows 2,3 with correct Int64 type
-- BUG: crashes with LOGICAL_ERROR: Bad cast from ColumnVector to ColumnVector
SELECT a, b FROM t_lwd_modify_fly ORDER BY a SETTINGS apply_mutations_on_fly = 1;

DROP TABLE t_lwd_modify_fly;
```

### Expected behavior

```
2 20
3 30
```

### Error message and/or stacktrace

```
Code: 49. DB::Exception: Bad cast from type DB::ColumnVector to DB::ColumnVector. (LOGICAL_ERROR)
```

### Additional context

**Open risks:**
- The broader pre-existing crash (DELETE + ALTER MODIFY + on-fly) masks this specific bug - even with the fix, the combination may still crash due to the general issue

**Suggested fix:** After line 102 in createLightweightDeleteCommand, add: mutation_command->mutation_version = command.mutation_version; (matching the pattern at line 59 in createCommandWithUpdatedColumns)

**Analysis details:** Confidence HIGH | Severity P1 | Testability: `STATELESS_SQL`

Found during automated review of [PR #79281](https://github.com/ClickHouse/ClickHouse/pull/79281).

---
_ClickGapAI · Confidence: HIGH · Severity: P1 · Finding: `h_pr79281_001`_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.