Implementation:ClickHouse ClickHouse Review Severity Model

Knowledge Sources	ClickHouse
Domains	Development_Process, Code_Quality
Last Updated	2026-02-08 00:00 GMT

Overview

Concrete tool for conducting structured ClickHouse code reviews using a severity model, risk checklist, and standardized output format defined in the project's review guidelines and contributing conventions.

Description

The ClickHouse Review Severity Model is a codified review pattern defined across two source files: `.github/copilot-instructions.md` (which serves as the canonical review guidelines for both human and AI reviewers) and `CONTRIBUTING.md` (which establishes project-wide contribution conventions). Together, these files define the complete review workflow, from initial risk scanning through finding classification to structured output.

The model consists of several interconnected components:

Review priorities (copilot-instructions.md, lines 21-36): A strict ordering of review concerns from most critical (correctness and safety) to least critical (ClickHouse-specific compliance), ensuring reviewers address the most dangerous issues first.

Risk checklist (copilot-instructions.md, lines 70-110): Six categories of bug patterns that reviewers must systematically scan for in every diff:

Memory and lifetime: raw pointer ownership, missing cleanup on early returns, dangling references/views, manual `new`/`delete` instead of RAII.
Resource management: unclosed file descriptors/sockets, leaks in loops, smart pointer misuse.
Concurrency and threading: unprotected shared state, lock ordering changes, non-thread-safe data structures, mutable globals.
Error handling and observability: ignored return values, cross-boundary exceptions, inconsistent error codes, missing failure logs.
Data correctness and serialization: format changes without versioning, missing migration logic, silent truncation/overflow.
Performance and algorithmic behavior: allocations in tight loops, unbounded data structures, accidental O(N^2) patterns, unnecessary syscalls.

Severity levels (copilot-instructions.md, lines 131-156): The three-tier classification system:

Blockers (must fix before merge): Incorrectness/data loss/corruption, memory/resource leaks or undefined behavior, new races or deadlocks, missing serialization versioning, unlogged deletion events, missing experimental gate for new features, significant performance regressions, security/privilege/license issues.
Majors (serious but not catastrophic): Under-tested edge cases, fragile code likely to break, hidden magic constants that should be settings, confusing user-visible behavior, missing comments in complex logic.
Nits (only if they materially improve robustness): Minor refactors reducing bug risk, small documentation improvements preventing user confusion. Explicitly excluded: typos, naming preferences, formatting, style.

Reviewer output format (copilot-instructions.md, lines 158-206): An eight-section structured report:

Summary: One paragraph explaining what the PR does and high-level verdict.
Missing context: Bullet list of critical information the reviewer lacked.
Findings (by severity): Blockers, Majors, and Nits with file/line references and suggested fixes.
Tests and Evidence: Coverage assessment, negative/error-handling test presence, guidance on additional tests.
ClickHouse Compliance Checklist: Yes/No answers for deletion logging, serialization versioning, experimental gates, settings exposure, backward compatibility, `SettingsHistory.cpp` updates, test preservation, documentation updates, core-area scrutiny.
Performance and Safety Notes: Hot-path implications, memory peaks, concurrency, failure modes, benchmarks.
User-Lens Review: Intuitiveness, robustness, actionable errors/logs.
Final Verdict: Approve, Request changes, or Block with minimum required actions.

Project conventions from `CONTRIBUTING.md` (lines 1-22):

CLA requirement: A bot prompts first-time contributors to sign the ClickHouse Individual CLA.
Never rebase or amend: All feedback must be addressed with new commits to preserve review history.
Developer instructions: Links to architecture documentation and build instructions.

Usage

This implementation is used during every code review cycle:

Initial review: The reviewer applies the risk checklist to the diff, classifies findings by severity, and produces the structured output format.
Re-review: After the contributor pushes new commits addressing feedback, the reviewer examines only the new commits (possible because rebase/amend is forbidden) and updates the finding status.
Merge decision: The reviewer issues a Final Verdict (Approve, Request changes, or Block) based on whether all Blockers and Majors have been resolved.

Code Reference

Source Location

Repository: ClickHouse
Review guidelines: .github/copilot-instructions.md (lines 1-259)
Contributing conventions: CONTRIBUTING.md (lines 1-22)

Signature

Review Severity Model:
  Blockers  -> Must fix before merge
  Majors    -> Serious, should normally fix
  Nits      -> Optional, only if materially improving robustness

Review Priority Order

1. Correctness & safety
2. Resource management
3. Concurrency & robustness
4. Performance characteristics
5. Maintainability & simplicity
6. User-facing quality
7. ClickHouse-specific compliance

I/O Contract

Inputs

Name	Type	Required	Description
PR diff	Git diff	Yes	The file-by-file diff of all changes in the pull request, including added, removed, and modified lines
PR title and description	Text	Yes	The pull request metadata including title, motivation, linked issues, and changelog information
CI status and logs	Status checks + logs	No	Results from the CI pipeline; if unavailable, noted under "Missing context"
Tests added/modified	Test files	No	New or modified test files included in the PR, with their results
Documentation changes	Doc files	No	User-facing documentation changes, release notes entries

Outputs

Name	Type	Description
Structured review report	Text (8 sections)	The complete review output following the defined format: Summary, Missing Context, Findings, Tests and Evidence, Compliance Checklist, Performance and Safety, User-Lens Review, Final Verdict
Finding classifications	Severity labels	Each finding tagged as Blocker, Major, or Nit with file/line references and suggested fixes
Final Verdict	Enum (Approve / Request changes / Block)	The merge decision with minimum required actions if not Approve
New commits from contributor	Git commits	The contributor's response to review findings, always as new commits (never rebase or amend)

Usage Examples

Example: Blocker finding

## Findings

### Blockers

- [src/Storages/MergeTree/MergeTreeDataWriter.cpp:245-260]
  The new code path deletes part metadata from ZooKeeper but does not log the
  deletion event. All data deletion events must be logged per ClickHouse policy.

  Suggested fix: Add LOG_INFO logging before the ZooKeeper delete call:
  LOG_INFO(log, "Deleting part metadata from ZooKeeper: {}", part_name);

Example: Major finding

### Majors

- [src/Functions/FunctionsConversion.cpp:1120-1135]
  The new conversion function handles the overflow case by silently truncating
  to zero. This will produce incorrect query results for large input values
  without any warning to the user.

  Suggested fix: Either throw an exception on overflow or add a setting to
  control overflow behavior (e.g., `conversion_overflow_mode`).

Example: Compliance checklist

## ClickHouse Compliance Checklist

- Data deletions logged? **No** - ZooKeeper node removal on line 258 is not logged.
- Serialization formats versioned? **N/A** - No format changes in this PR.
- Experimental setting gate present? **Yes** - Gated behind `allow_experimental_vector_search`.
- Settings exposed for constants/thresholds? **Yes** - `vector_search_max_candidates` added.
- Backward compatibility preserved? **Yes** - Default behavior unchanged.
- SettingsHistory.cpp updated? **Yes** - New settings added with version 25.2.
- Existing tests untouched? **Yes** - Only new tests added.
- Docs updated? **No** - Documentation checkbox unchecked; needs docs for the new setting.
- Core-area extra scrutiny? **Yes** - Changes touch MergeTree storage engine.

Example: Addressing review feedback (contributor)

# CORRECT: Add a new commit to address review feedback
git add src/Storages/MergeTree/MergeTreeDataWriter.cpp
git commit -m "Add deletion logging for ZooKeeper part metadata removal"
git push

# INCORRECT: Never do this in the ClickHouse project
# git commit --amend   # FORBIDDEN
# git rebase -i HEAD~3 # FORBIDDEN

Example: Final verdict

## Final Verdict

**Status: Request changes**

Minimum required actions:
1. Add deletion logging for the ZooKeeper metadata removal (Blocker).
2. Write documentation for the new `vector_search_filter_mode` setting (Major).

Related Pages

Page Connections

Double-click a node to navigate. Hold to expand connections.

Principle

Implementation

Heuristic

Environment