Fix UNKNOWN_IDENTIFIER on ALTER TABLE ... DROP COLUMN with aliased default expression#109374
Fix UNKNOWN_IDENTIFIER on ALTER TABLE ... DROP COLUMN with aliased default expression#109374alexey-milovidov wants to merge 2 commits into
Conversation
…fault expression When a column has a `DEFAULT` or `MATERIALIZED` expression that defines and later references an inline alias (e.g. `f(...) AS a, ..., a`), `ALTER TABLE ... DROP COLUMN` of an unrelated column failed with `UNKNOWN_IDENTIFIER`. The column-dependency check performed for the dropped column resolves each remaining default expression as a standalone query tree node via `QueryAnalyzer::resolve`. That function collected inline aliases into the resolution scope only when the node was a `LIST`, not when it was a single expression node (which is what `buildQueryTree` produces for a bare default expression). As a result the internal alias was never registered and the later reference to it could not be resolved, while the same expression validates fine at `CREATE TABLE` time (where it is wrapped in a query node). Collect the aliases for the single-node case as well, making standalone expression resolution consistent with list resolution. Closes: #109370 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clickhouse-gh
Bot
added
the
pr-bugfix
`lowerUTF8` requires ICU, which is excluded from the Fast test build, so the test failed there with `Function with name 'lowerUTF8' does not exist`. The function is applied to `hex(MD5(url))`, which is ASCII hex, so `lower` produces an identical result. It is used in the `MATERIALIZED md5` column, which is dropped by the test, so the expected output is unchanged. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109374&sha=4fdec8aa98df6b893755f3590d0dbd8a1b05f465&name_0=PR&name_1=Fast%20test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| /// a single expression node too, not only for a list: otherwise an alias defined and later | ||
| /// referenced within a standalone expression (such as a column DEFAULT expression checked | ||
| /// during `ALTER TABLE ... DROP COLUMN`) is not found and resolution fails with UNKNOWN_IDENTIFIER. | ||
| QueryExpressionsAliasVisitor visitor(scope.aliases); |
There was a problem hiding this comment.
resolve now collects inline aliases for bare expression nodes, but resolveConstantExpression immediately below still does not. That entrypoint is what evaluateConstantExpression and StorageMergeTreeAnalyzeIndexes use, so shapes like LIMIT plus(1 AS a, a) or numbers(plus(5 AS x, x)) can still fail with UNKNOWN_IDENTIFIER even after this PR. If the intended invariant is “standalone expressions resolve like lists”, the same QueryExpressionsAliasVisitor pre-pass needs to happen in resolveConstantExpression too.
There was a problem hiding this comment.
I looked into this and believe it's a false positive — I couldn't construct any user-facing failure through resolveConstantExpression.
The two named cases don't reproduce, because they go through full query-tree resolution (resolveQuery / resolveTableFunction, which already run QueryExpressionsAliasVisitor), not resolveConstantExpression:
SELECT number FROM numbers(10) LIMIT plus(1 AS a, a)→ 2 rowsSELECT count() FROM numbers(plus(5 AS x, x))→ 10
Exercising resolveConstantExpression directly (a parameterized-view argument goes FunctionParameterValuesVisitor → evaluateConstantExpressionOrIdentifierAsLiteral → resolveConstantExpression) works for forward, backward, and nested inline aliases — including the exact nested shape that broke ALTER ... DROP COLUMN:
pv(n = plus(2 AS a, a))→ okpv(n = plus(a, 5 AS a))(backward reference) → okpv(n = length(concat(substring(upper('abc') AS u, 1, 3), '-', u)))→ ok
The three resolveConstantExpression callers are evaluateConstantExpression (its fake table has only _dummy), StorageMergeTreeAnalyzeIndexes, and InverseDictionaryLookupPass (the latter two build nodes internally, not from user-written aliases), so none reaches the shape that fails via resolve. Mirroring the pre-pass there would be harmless for consistency/defense-in-depth, but it isn't needed for correctness. Leaving the call to you.
(Verified on a pre-fix build; resolveConstantExpression is unchanged by this PR.)
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered: 6/6 (100.00%) · Uncovered code |
1 participant
Closes: #109370
When a column has a
DEFAULTorMATERIALIZEDexpression that defines and later references an inline alias (for examplef(...) AS a, ..., a), anALTER TABLE ... DROP COLUMNof an unrelated column failed withUNKNOWN_IDENTIFIER, even though the very same expression is accepted atCREATE TABLEtime.The column-dependency check that runs before dropping a column resolves each remaining default expression as a standalone query-tree node via
QueryAnalyzer::resolve. That function collected inline aliases into the resolution scope only when the node was aLIST, but not when it was a single expression node — which is exactly whatbuildQueryTreeproduces for a bare default expression. As a result the internal alias was never registered in the scope and the later reference to it could not be resolved.The fix collects the aliases for the single-expression case as well, making standalone expression resolution consistent with list resolution (the path already used by
CREATE TABLEvalidation).A new stateless test
04502_alter_drop_column_default_with_aliascovers both a simple aliased default and the nested-lambda case from the report.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix
UNKNOWN_IDENTIFIERerror onALTER TABLE ... DROP COLUMNwhen another column has aDEFAULTorMATERIALIZEDexpression that defines and references an inline alias.