Skip to content

Fix logical error in ALTER TABLE DROP PART with non-literal AST#99489

Merged
alexey-milovidov merged 2 commits intomasterfrom
fix-ast-fuzzer-bad-cast-part-name
Mar 16, 2026
Merged

Fix logical error in ALTER TABLE DROP PART with non-literal AST#99489
alexey-milovidov merged 2 commits intomasterfrom
fix-ast-fuzzer-bad-cast-part-name

Conversation

@alexey-milovidov
Copy link
Member

Summary

  • Fix logical error (server abort in ASan builds) when ALTER TABLE ... DROP PART receives a non-literal AST node — e.g. via a typed query parameter like {partition:Date} which the server substitutes with _CAST(value, 'Type')
  • Replace all 8 unsafe as<ASTLiteral &>() reference casts in part name extraction with safe pointer casts that throw BAD_ARGUMENTS
  • Affects MergeTreeData.cpp (6 locations) and StorageReplicatedMergeTree.cpp (2 locations)

Closes #99401

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=bb7e748bbc81a5f9181f4fdb5f80aa1c6180c0a7&name_0=MasterCI&name_1=AST%20fuzzer%20%28arm_asan%29

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fix logical error in ALTER TABLE ... DROP PART when a typed query parameter is used for the part name.

🤖 Generated with Claude Code

The AST fuzzer found that when a query parameter with a non-String type
(e.g. `{partition:Date}`) is used in `ALTER TABLE ... DROP PART`, the
server replaces it with `_CAST(value, 'Type')` which is an `ASTFunction`,
not an `ASTLiteral`. The code then called `as<ASTLiteral &>()` which
threw a logical error (abort in debug/ASan builds) instead of a proper
user-facing exception.

Replace all unsafe `as<ASTLiteral &>()` reference casts in part name
extraction with a safe pointer cast that throws `BAD_ARGUMENTS` on
type mismatch. This affects 8 locations across `MergeTreeData.cpp` and
`StorageReplicatedMergeTree.cpp`.

https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=bb7e748bbc81a5f9181f4fdb5f80aa1c6180c0a7&name_0=MasterCI&name_1=AST%20fuzzer%20%28arm_asan%29

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

clickhouse-gh bot commented Mar 14, 2026

Workflow [PR], commit [a4ec8db]

Summary:


AI Review

Summary

This PR fixes a real correctness issue where ALTER TABLE ... DROP PART could throw an internal logical error when the partition argument is represented as a non-literal AST node (for example, typed query parameters expanded to _CAST(...)). The patch replaces unsafe AST downcasts with explicit validation and returns a user-facing BAD_ARGUMENTS exception instead. Coverage includes both MergeTreeData and replicated paths touched by this pattern, and adds a stateless regression test for the reported case.

ClickHouse Rules

Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsHistory.cpp
Safe rollout
Compilation time

Final Verdict

  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 14, 2026
- Add `SET param_partition` so the query parameter substitution happens
  (previously got `UNKNOWN_QUERY_PARAMETER` instead of `BAD_ARGUMENTS`)
- Rename test from 04039 to 04040 to avoid conflict with existing tests

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

clickhouse-gh bot commented Mar 16, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.70% 83.70% +0.00%
Functions 23.90% 23.90% +0.00%
Branches 76.20% 76.20% +0.00%

PR changed lines: PR changed-lines coverage: 82.35% (42/51, 0 noise lines excluded)
Diff coverage report
Uncovered code

Copy link
Member Author

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

That's a good one!

@alexey-milovidov alexey-milovidov self-assigned this Mar 16, 2026
@alexey-milovidov alexey-milovidov merged commit 2deb66f into master Mar 16, 2026
162 of 163 checks passed
@alexey-milovidov alexey-milovidov deleted the fix-ast-fuzzer-bad-cast-part-name branch March 16, 2026 21:34
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 16, 2026
zlareb1 added a commit to zlareb1/ClickHouse that referenced this pull request Mar 17, 2026
…eter (PR ClickHouse#99489)

PR ClickHouse#99489 introduced `getPartNameFromAST` to replace direct `as<ASTLiteral&>()`
casts. The PR's own test (`04040`) covers `DROP PART`; the `dropDetached` code
path at `MergeTreeData.cpp:7706` was equally affected but not tested.

A query parameter `{partition:Date}` is replaced by `_CAST(...)` — an
`ASTFunction` — so the old code threw `LOGICAL_ERROR` (bad cast). The fix
throws `BAD_ARGUMENTS` with a helpful message.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 19, 2026
…non-literal

Add test for DROP DETACHED PART with non-String typed query parameter (PR #99489)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default 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.

Logical error: Bad cast from type A to B (STID: 1635-3b8c)

2 participants