Skip to content

Fix local server crash on CREATE DICTIONARY with bogus definition.#100036

Merged
yakov-olkhovskiy merged 2 commits intomasterfrom
fix-local-create-dictionary
Mar 20, 2026
Merged

Fix local server crash on CREATE DICTIONARY with bogus definition.#100036
yakov-olkhovskiy merged 2 commits intomasterfrom
fix-local-create-dictionary

Conversation

@yakov-olkhovskiy
Copy link
Copy Markdown
Member

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 local server crash when CREATE DICTIONARY has a definition with list value containing non-existing function.

closes #99995

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 19, 2026

Workflow [PR], commit [439aa5e]

Summary:


AI Review

Summary

This PR fixes a null dereference in buildConfigurationFromFunctionWithKeyValueArguments when loading dictionary definitions that contain an unknown function in a key-value list. The added guard converts that path into INCORRECT_DICTIONARY_DEFINITION, and the new stateless test reproduces and validates the behavior through clickhouse-local. High-level verdict: the fix is correct, targeted, and test-covered.

Missing context

  • ⚠️ Full CI logs/Praktika report details were not provided in this review context.

ClickHouse Rules

Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.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 19, 2026
Copy link
Copy Markdown
Member

@evillique evillique left a comment

Choose a reason for hiding this comment

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

Should we add a simple one-line SQL test?

@evillique evillique self-assigned this Mar 19, 2026
Comment thread src/Dictionaries/getDictionaryConfigurationFromAST.cpp
cursor Bot pushed a commit that referenced this pull request Mar 19, 2026
…unction

The existing test 03290_dictionary_assert_on_function.sql runs against
clickhouse-server where the early `isServerCompletelyStarted` check fires
before reaching the null-pointer dereference. This new test exercises the
fix through clickhouse-local, where the application type is LOCAL (not
SERVER), so the early check is skipped and the new null-check code path
in `buildConfigurationFromFunctionWithKeyValueArguments` is exercised.

Ref: #100036

Co-authored-by: Yakov Olkhovskiy <yakov-olkhovskiy@users.noreply.github.com>
…unction

The existing test 03290_dictionary_assert_on_function.sql runs against
clickhouse-server where the early `isServerCompletelyStarted` check fires
before reaching the null-pointer dereference. This new test exercises the
fix through clickhouse-local, where the application type is LOCAL (not
SERVER), so the early check is skipped and the new null-check code path
in `buildConfigurationFromFunctionWithKeyValueArguments` is exercised.

Ref: #100036

Co-authored-by: Yakov Olkhovskiy <yakov-olkhovskiy@users.noreply.github.com>
@yakov-olkhovskiy
Copy link
Copy Markdown
Member Author

Should we add a simple one-line SQL test?

yeah, delegated it to AI :)

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ yakov-olkhovskiy
❌ cursoragent
You have signed the CLA already but the status is still pending? Let us recheck it.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 19, 2026

LLVM Coverage Report

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

PR changed lines: PR changed-lines coverage: 80.00% (8/10, 0 noise lines excluded)
Diff coverage report
Uncovered code

@yakov-olkhovskiy yakov-olkhovskiy added this pull request to the merge queue Mar 20, 2026
Merged via the queue into master with commit 99ba7a3 Mar 20, 2026
321 of 322 checks passed
@yakov-olkhovskiy yakov-olkhovskiy deleted the fix-local-create-dictionary branch March 20, 2026 20:48
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 20, 2026
@clickgapai
Copy link
Copy Markdown
Contributor

Hi @yakov-olkhovskiy @evillique — while reviewing this PR I found the following:

Happy to discuss — close anything that's wrong or already addressed.

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.

Local server crashes on CREATE DICTIONARY with bogus definition. LibFuzzer finding.

6 participants