Skip to content

Allow use_partition_pruning setting to also disable MinMax index on partition key columns#100904

Merged
nihalzp merged 6 commits intoClickHouse:masterfrom
nihalzp:improve-setting-use-partition-pruning
Apr 1, 2026
Merged

Allow use_partition_pruning setting to also disable MinMax index on partition key columns#100904
nihalzp merged 6 commits intoClickHouse:masterfrom
nihalzp:improve-setting-use-partition-pruning

Conversation

@nihalzp
Copy link
Copy Markdown
Member

@nihalzp nihalzp commented Mar 27, 2026

This will be useful to also detect bugs due to partition's minmax index condition such as:

CREATE TABLE t (x Int32) ENGINE = MergeTree PARTITION BY x ORDER BY tuple();
INSERT INTO t VALUES (0), (1), (2);

SELECT * FROM t WHERE x != 0 AND intDiv(100, x) > 10; -- throws Division by zero. (ILLEGAL_DIVISION)

We can now see that it was because of partition's minmax index condition:

CREATE TABLE t (x Int32) ENGINE = MergeTree PARTITION BY x ORDER BY tuple();
INSERT INTO t VALUES (0), (1), (2);

SELECT * FROM t WHERE x != 0 AND intDiv(100, x) > 10 SETTINGS use_partition_pruning = 0; -- works

Changelog category (leave one):

  • Improvement

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

use_partition_pruning = 0 now also disables MinMax index pruning and count optimization on partition key columns, in addition to disabling pruning based on partition keys.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 27, 2026

Workflow [PR], commit [77eb3da]

Summary:

job_name test_name status info comment
Stress test (amd_asan_ubsan) failure
Logical error: Column identifier A is already registered (STID: 4697-369d) FAIL cidb, issue ISSUE EXISTS
Stress test (arm_msan) failure
MemorySanitizer: use-of-uninitialized-value (STID: 2410-4d7c) FAIL cidb IGNORED
Stress test (arm_ubsan) failure
Logical error: Shard number is greater than shard count: shard_num=A shard_count=B cluster=C (STID: 5066-457d) FAIL cidb IGNORED

AI Review

Summary

This PR makes use_partition_pruning = 0 disable both partition pruning and partition-key MinMax pruning (including the _minmax_count_projection path), and updates tests accordingly. The core code changes are coherent and align behavior across ReadFromMergeTree and MergeTreeData. I found one major gap: tests do not cover the concrete exception scenario from the PR motivation (intDiv with x = 0), so the key regression claim is not directly verified.

Findings

⚠️ Majors

  • [tests/queries/0_stateless/04062_use_partition_pruning_disable_minmax_count_projection.sql:26] The new test validates explain-plan and result correctness, but it does not reproduce the exact exception path from the PR description (x != 0 AND intDiv(100, x) > 10 with a partition containing x = 0). Without this, the central regression/fix claim is only indirectly tested.
    • Suggested fix: add a focused regression case that inserts 0, verifies exception with use_partition_pruning = 1 (serverError 153), and verifies success with use_partition_pruning = 0.
Tests

⚠️ Add one stateless regression test scenario for x != 0 AND intDiv(100, x) > 10 (partition includes x = 0) to directly validate that disabling use_partition_pruning prevents the ILLEGAL_DIVISION exception while preserving correct results.

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
Final Verdict
  • Status: ⚠️ Request changes
  • Minimum required actions:
    • Add the missing regression test for the concrete ILLEGAL_DIVISION scenario described in the PR body.

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Mar 27, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 30, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.20% +0.10%
Functions 90.90% 90.80% -0.10%
Branches 76.70% 76.80% +0.10%

Changed lines: 100.00% (31/31) · Uncovered code

Full report · Diff report

@azat azat self-assigned this Mar 31, 2026
@nihalzp nihalzp added this pull request to the merge queue Apr 1, 2026
Merged via the queue into ClickHouse:master with commit bfc23b0 Apr 1, 2026
160 of 164 checks passed
@nihalzp nihalzp deleted the improve-setting-use-partition-pruning branch April 1, 2026 08:22
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 1, 2026
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 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.

3 participants