Skip to content

Follow up on : Allow certain functions without parentheses in SQL #95949#98941

Merged
zlareb1 merged 7 commits intoClickHouse:masterfrom
AlyHKafoury:revert-95943-revert-94678-Allow-certain-functions-without-parentheses-in-SQL
Mar 9, 2026
Merged

Follow up on : Allow certain functions without parentheses in SQL #95949#98941
zlareb1 merged 7 commits intoClickHouse:masterfrom
AlyHKafoury:revert-95943-revert-94678-Allow-certain-functions-without-parentheses-in-SQL

Conversation

@AlyHKafoury
Copy link
Copy Markdown
Contributor

@AlyHKafoury AlyHKafoury commented Mar 6, 2026

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Note

Medium Risk
Touches core identifier/alias resolution in QueryAnalyzer, which can subtly change query semantics (especially around AS aliases vs niladic functions) across SELECT/DEFAULT/INSERT paths.

Overview
Fixes alias-based identifier resolution so when an alias points to a FUNCTION node, resolveExpressionNode is called with the correct ignore_alias and allow_niladic_functions arguments, preventing AS aliases from interfering with niladic function resolution.

Extends 03800_functions_without_parentheses to cover multiple niladic functions (e.g. DATABASE, SCHEMA, USER, CURDATE, CURRENT_TIMESTAMP) and to validate behavior in DEFAULT expressions and INSERT ... SELECT flows, with updated expected outputs.

Written by Cursor Bugbot for commit 72fc2fa. This will update automatically on new commits. Configure here.

@AlyHKafoury
Copy link
Copy Markdown
Contributor Author

@novikd @alexey-milovidov Can you enable CI please,

CC : @zlareb1

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 6, 2026

Workflow [PR], commit [72fc2fa]

Summary:

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 6, 2026
@zlareb1 zlareb1 added the can be tested Allows running workflows for external contributors label Mar 6, 2026
@AlyHKafoury
Copy link
Copy Markdown
Contributor Author

@zlareb1 @novikd I believe this is good to merge

@zlareb1 zlareb1 self-assigned this Mar 8, 2026
@AlyHKafoury
Copy link
Copy Markdown
Contributor Author

@zlareb1 merged the tests in 1 file, and tests passed I believe it is ready to merge.

@zlareb1 zlareb1 enabled auto-merge March 9, 2026 08:41
@AlyHKafoury
Copy link
Copy Markdown
Contributor Author

@zlareb1 I cannot see what is CH Inc sync ? should I retrigger CI or is it something that requires your input ?

@novikd novikd self-assigned this Mar 9, 2026
Copy link
Copy Markdown
Member

@novikd novikd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

auto-merge was automatically disabled March 9, 2026 14:59

Head branch was pushed to by a user without write access

@AlyHKafoury AlyHKafoury requested a review from zlareb1 March 9, 2026 15:00
@AlyHKafoury
Copy link
Copy Markdown
Contributor Author

@zlareb1 I fixed comment by the AI, can you please reenabled auto merge ?

@zlareb1 zlareb1 enabled auto-merge March 9, 2026 18:37
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 9, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.20% 83.80% +0.60%
Functions 22.50% 23.90% +1.40%
Branches 75.30% 76.30% +1.00%

PR changed lines: PR changed-lines coverage: 100.00% (7/7)
Diff coverage report
Uncovered code

@zlareb1 zlareb1 added this pull request to the merge queue Mar 9, 2026
Merged via the queue into ClickHouse:master with commit 1f984e3 Mar 9, 2026
163 checks passed
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 9, 2026
zlareb1 added a commit to zlareb1/ClickHouse that referenced this pull request Mar 17, 2026
…NG, CASE, and JOIN contexts (PR ClickHouse#98941)

PR ClickHouse#98941 fixed a bug where `allow_to_resolve_niladic_functions` was incorrectly passed as `ignore_alias` in QueryAnalyzer.cpp:1083. The existing test (03800_functions_without_parentheses) only covered niladic functions in SELECT expressions, WHERE, DEFAULT, and INSERT-SELECT. This extends the test to cover the additional SQL contexts where `tryResolveIdentifier` is called with this flag: scalar subqueries, GROUP BY, ORDER BY, HAVING, CASE expressions, and JOIN conditions.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
zlareb1 added a commit to zlareb1/ClickHouse that referenced this pull request Mar 17, 2026
…se#98941 fix

Replace the chained-alias test case with `WITH toDate(TODAY) AS d SELECT d = today()`,
which places a niladic identifier directly inside a function alias body. This is the
most direct regression test for the bug fixed in ClickHouse#98941: `allow_niladic_functions` was
inadvertently passed as `false` when resolving a FUNCTION-type alias node at
`QueryAnalyzer.cpp:1084`, so niladic identifiers inside such alias bodies could not
resolve and would throw `UNKNOWN_IDENTIFIER`.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog 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.

4 participants