Skip to content

Refactor projection settings: replace optional fields with generic SettingsChanges#101170

Open
amosbird wants to merge 1 commit intoClickHouse:masterfrom
amosbird:projection-index-generic
Open

Refactor projection settings: replace optional fields with generic SettingsChanges#101170
amosbird wants to merge 1 commit intoClickHouse:masterfrom
amosbird:projection-index-generic

Conversation

@amosbird
Copy link
Copy Markdown
Collaborator

Replace per-projection optional index_granularity/index_granularity_bytes fields with a generic SettingsChanges map and a has_index_granularity_overrides flag. This enables projections to override any allowed MergeTreeSettings, not just granularity.

Changes:

  • ProjectionDescription: replace optional<UInt64> index_granularity/index_granularity_bytes with SettingsChanges settings_changes and bool has_index_granularity_overrides. Remove loadSettings; validation now happens in getProjectionFromAST.
  • MergeTreeData::getSettings: accept const SettingsChanges * instead of ProjectionDescriptionRawPtr. Skip copy when settings_changes is empty.
  • IProjectionIndex: add getDefaultSettings and getMaxRows virtual methods.
  • getProjectionFromAST: accept LoadingStrictnessLevel. On CREATE, enforce an ALLOWED_PROJECTION_SETTINGS whitelist covering part format, granularity, compression, serialization, and related settings. Run sanityCheck and reject index_granularity_bytes = 0.
  • Add test 04068_projection_index_settings covering granularity overrides, ALTER+MATERIALIZE, DETACH/ATTACH persistence, min_bytes_for_wide_part override (Compact vs parent), min_compress_block_size override, disallowed settings rejection, and non-adaptive table rejection.

Changelog category (leave one):

  • Improvement

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

Allow projections to override additional MergeTree settings (compression, part format, serialization) beyond just index_granularity, with a whitelist of allowed settings and validation.

Documentation entry for user-facing changes

  • Documentation is not required (internal refactor, no new user-facing syntax)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 30, 2026

Workflow [PR], commit [89468aa]

Summary:

job_name test_name status info comment
AST fuzzer (amd_debug) failure
Logical error: Primary key type mismatch, expected A, got B. (STID: 3436-4175) FAIL cidb

AI Review

Summary

This PR refactors projection-level settings to use generic SettingsChanges, wires these overrides through MergeTree settings resolution, and adds validation plus stateless coverage for projection setting persistence and behavior. I reviewed the touched core storage/projection code paths and tests and did not find additional high-confidence correctness, safety, or performance issues beyond an existing inline bot comment already posted on this PR.

Missing context
  • ⚠️ No CI test logs or performance report artifacts were provided in this review task, so runtime validation relied on code inspection and added test coverage in the diff.
ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
No large/binary files
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Mar 30, 2026
@amosbird amosbird marked this pull request as ready for review March 30, 2026 05:40
@KochetovNicolai KochetovNicolai self-assigned this Mar 30, 2026
amosbird added a commit to amosbird/ClickHouse that referenced this pull request Apr 2, 2026
…ttingsChanges

Replace per-projection optional index_granularity/index_granularity_bytes
fields with a generic SettingsChanges map and has_index_granularity_overrides
flag. Projections can now override any allowed MergeTreeSettings via a
whitelist (part format, granularity, compression, serialization).
Adds getDefaultSettings() and getMaxRows() to IProjectionIndex interface.

Ref: ClickHouse#101170
amosbird added a commit to amosbird/ClickHouse that referenced this pull request Apr 2, 2026
…ttingsChanges

Replace per-projection optional index_granularity/index_granularity_bytes
fields with a generic SettingsChanges map and has_index_granularity_overrides
flag. Projections can now override any allowed MergeTreeSettings via a
whitelist (part format, granularity, compression, serialization).
Adds getDefaultSettings() and getMaxRows() to IProjectionIndex interface.

Ref: ClickHouse#101170
@alexey-milovidov
Copy link
Copy Markdown
Member

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

Comment thread src/Storages/ProjectionsDescription.cpp
@alexey-milovidov
Copy link
Copy Markdown
Member

The Can't adjust last granule error in CI is a known issue. The fix is in #101641

@amosbird amosbird force-pushed the projection-index-generic branch 2 times, most recently from 0bf625d to 56f6afb Compare April 13, 2026 03:20
@clickhouse-gh clickhouse-gh Bot added the submodule changed At least one submodule changed in this PR. label Apr 13, 2026
…ttingsChanges

Replace per-projection `optional<UInt64> index_granularity/index_granularity_bytes`
fields with a generic `SettingsChanges` map and `has_index_granularity_overrides` flag.
Projections can now override any allowed MergeTreeSettings via a whitelist
(part format, granularity, compression, serialization).

Adds `getDefaultSettings` and `getMaxRows` to `IProjectionIndex` interface.

The `index_granularity_bytes != 0` check is enforced unconditionally (both
CREATE and ATTACH paths) to prevent projections from falling back to fixed
granularity behavior.
@amosbird amosbird force-pushed the projection-index-generic branch from 56f6afb to 89468aa Compare April 13, 2026 03:25
@amosbird amosbird removed the submodule changed At least one submodule changed in this PR. label Apr 13, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 13, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.00% -0.10%
Functions 90.90% 90.90% +0.00%
Branches 76.60% 76.50% -0.10%

Changed lines: 91.60% (109/119) | lost baseline coverage: 4 line(s) · Uncovered code

Full report · Diff report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants