Skip to content

Support tuple lexicographic comparison for index analysis - #108290

Open
fastio wants to merge 9 commits into
ClickHouse:masterfrom
fastio:feature-tuple-pk-index-scan
Open

Support tuple lexicographic comparison for index analysis#108290
fastio wants to merge 9 commits into
ClickHouse:masterfrom
fastio:feature-tuple-pk-index-scan

Conversation

@fastio

@fastio fastio commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Issue #75086.

Use lexicographic tuple comparisons on key columns, e.g. (x, y) < (1, 5), for granule pruning. Previously such conditions were dropped to FUNCTION_UNKNOWN and read all granules, while x < 1 OR (x = 1 AND y < 5) and (x, y) IN (...) worked.

Gated by the new setting analyze_index_with_tuple_lexicographic_comparison (default true). Benefits primary key, part-level minmax, and minmax skip indexes.

Example:

CREATE TABLE t (a UInt32, b UInt32) ENGINE = MergeTree ORDER BY (a, b)
SETTINGS index_granularity = 4;
INSERT INTO t SELECT intDiv(number, 16), intDiv(number, 4) % 4 FROM numbers(64);

EXPLAIN indexes = 1 SELECT count() FROM t WHERE (a, b) < (2, 1);

Before:

Condition: true
Granules: 16/16

See: https://fiddle.clickhouse.com/46ac4c2d-eb71-4bd7-9ea3-0fa61a5c83fc

After:

Condition: ((a, b) < (2, 1))
Granules: 9/16

Changelog category (leave one):

  • Improvement

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

Use lexicographic tuple comparisons on key columns, e.g. (a, b) < (1, 5), for primary key and minmax index granule pruning. Controlled by the new setting analyze_index_with_tuple_lexicographic_comparison (enabled by default).

Documentation entry for user-facing changes

  • Documentation is written (the new setting is documented in src/Core/Settings.cpp)

Let KeyCondition use (k1, k2, ...) <op> (c1, c2, ...) for primary key and minmax
granule pruning, gated by analyze_index_with_tuple_lexicographic_comparison.
@fastio
fastio marked this pull request as draft June 23, 2026 12:25
@nihalzp nihalzp self-assigned this Jun 23, 2026
@nihalzp nihalzp added the can be tested Allows running workflows for external contributors label Jun 23, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [1e6415e]

Summary:

job_name test_name status info comment
Fast test (arm_darwin) FAIL
01548_parallel_parsing_max_memory FAIL cidb
Stateless tests (arm_asan_ubsan, targeted) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Too many test failures FAIL cidb
Stateless tests (amd_asan_ubsan, flaky check) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Too many test failures FAIL cidb
Stateless tests (amd_tsan, flaky check) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Too many test failures FAIL cidb
Stateless tests (amd_msan, flaky check) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Too many test failures FAIL cidb
Stateless tests (amd_debug, flaky check) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Too many test failures FAIL cidb
Stateless tests (amd_tsan, parallel) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Stateless tests (amd_msan, WasmEdge, parallel, 2/2) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Stateless tests (amd_tsan, s3 storage, parallel, 1/3) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb
Stateless tests (arm_binary, parallel) FAIL
04545_tuple_lexicographic_nan_index_pruning FAIL cidb

AI Review

Summary

This PR teaches KeyCondition to prune on lexicographic tuple comparisons such as (a, b) < (1, 5) for primary-key and minmax analysis, behind analyze_index_with_tuple_lexicographic_comparison. The basic dense/sparse/minmax paths are in place and CI is green, but there is still one new correctness issue on tuple monotonic chains over LowCardinality keys, plus two older tuple-atom gaps that are still present in the current code.

Findings

❌ Blockers

  • [src/Storages/MergeTree/KeyCondition.cpp:6302] The tuple monotonic-chain branch no longer mirrors the scalar FUNCTION_IN_RANGE handling for LowCardinality keys. isKeyPossiblyWrappedByMonotonicFunctions() builds each chain against the nested type, but this branch passes raw data_types[kc] / sparse_data_types[sparse_pos] into applyMonotonicFunctionsChainToRange(). With a predicate like (CAST(lc_key AS Int32), id) < (...), that can drive the chain through the same LowCardinality bad-cast path the scalar code already avoids by stripping LC first. Because hasMonotonicFunctionsChain() still ignores tuple_lexicographic_chains, this is reachable on ordinary PK reads today instead of only on fallback paths.
    Suggested fix: pass recursiveRemoveLowCardinality(...) in both tuple branches, matching the scalar range path, and add a focused test with a LowCardinality key plus a monotonic wrapper such as CAST.

⚠️ Majors

  • [src/Storages/MergeTree/KeyCondition.cpp:7412] hasMonotonicFunctionsChain() still only looks at element.monotonic_functions_chain, so tuple atoms with per-element chains never switch create_field_ref() to the cached block-backed path. Queries like (toDate(t), id) < (...) therefore materialize every PK boundary as standalone Fields and re-run the monotonic function per boundary instead of reusing the transformed column, which undercuts the hot-path speedup this PR is trying to add.
    Suggested fix: treat non-empty tuple_lexicographic_chains the same way as monotonic_functions_chain when deciding whether the condition carries monotonic work.

  • [src/Storages/MergeTree/KeyCondition.cpp:5903] The tuple corner comparator still treats the encoded nullable-key sentinel (POSITIVE_INFINITY) as an ordinary ordered value. A single-row granule like (4, NULL) is therefore classified as definitely satisfying (a, b) >= (4, 3), even though the SQL tuple comparison is NULL and should not match. I do not see an ordinary row-dropping can_be_true caller today because tuple atoms are excluded from matchesExactContinuousRange(), but the atom’s BoolMask is still non-conservative and any exact-range / count-style consumer will inherit a wrong can_be_false.
    Suggested fix: treat encoded-null bounds as indeterminate here, or relax / disable the tuple atom whenever a later compared key column may still be NULL.

Final Verdict

Changes requested.

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jun 23, 2026
Comment thread src/Core/SettingsChangesHistory.cpp Outdated
Comment thread src/Storages/MergeTree/KeyCondition.cpp
fastio added 2 commits June 24, 2026 22:14
The missing terminator swallowed the next DECLARE, breaking the
link; also record the history entry as false->true for compatibility.
Add the missing branch to the sparse checkInHyperrectangle overload,
factoring the corner comparison into a shared helper to avoid an exception.
Comment thread src/Storages/MergeTree/KeyCondition.cpp Outdated
@fastio
fastio marked this pull request as ready for review July 15, 2026 10:00
@clickhouse-gh

clickhouse-gh Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Dear @nihalzp, you haven't been active on this PR for 30 days. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

@nihalzp nihalzp self-assigned this Jul 24, 2026
fastio added 2 commits July 29, 2026 15:32
Record `analyze_index_with_tuple_lexicographic_comparison` under the current settings-history version and pin the tuple-index test to legacy `EXPLAIN` output.

CI: ClickHouse#108290
const bool included = use_lower ? r.left_included : r.right_included;
const Field & ci = c[i];

if (isRealNull(bound) || isRealNull(ci) || bound.isNaN() || ci.isNaN())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

compareTupleCorner only treats a literal Field::Null as indeterminate, but primary-key bounds never arrive here that way: MergeTreeDataSelectExecutor::create_field_ref rewrites nullable key values to POSITIVE_INFINITY before checkInRange runs. On a mark whose only tuple is (4, NULL), this code therefore reports (a, b) >= (4, 3) as definitely true (lower == upper == Greater), even though tuple comparison with a later NULL is NULL and should not match. That makes the new tuple atom non-conservative on nullable keys and gives an unsound can_be_false to exact-range users. Please either treat encoded-NULL bounds as indeterminate here or relax / disable the tuple atom whenever a later compared key column can still be NULL.

out.function = RPNElement::FUNCTION_TUPLE_LEXICOGRAPHIC;
out.key_columns = std::move(mapped_key_columns);
out.tuple_lexicographic_constant = std::move(mapped_constant);
out.tuple_lexicographic_chains = std::move(mapped_chains);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These new per-element monotonic chains never make KeyCondition::hasMonotonicFunctionsChain() return true, because that helper still only checks element.monotonic_functions_chain. The result is that MergeTreeDataSelectExecutor::create_field_ref() takes the explicit-Field path for tuple predicates, so applyMonotonicFunctionsChainToRange() executes functions like toDate separately for every boundary instead of using the cached block-backed FieldRef path that scalar predicates get. The new functional-tuple case regresses PK analysis exactly on the hot path this optimization adds; hasMonotonicFunctionsChain() needs to look at tuple_lexicographic_chains too.

if (i < chains.size() && !chains[i].empty())
{
std::optional<Range> new_range
= applyMonotonicFunctionsChainToRange(range, chains[i], data_types[kc], single_point);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The scalar FUNCTION_IN_RANGE path strips LowCardinality before calling applyMonotonicFunctionsChainToRange() (recursiveRemoveLowCardinality(data_types[key_column]) a few lines above), but the tuple branch feeds the raw data_types[kc] / sparse_data_types[sparse_pos] back into the same helper. That reintroduces the mismatch the scalar path already had to harden against: the chain is built against the nested type, so a tuple predicate like (CAST(lc_key AS Int32), id) < (...) can hit the LowCardinality bad-cast path here instead of behaving like the scalar equivalent. This is reachable today because hasMonotonicFunctionsChain() still ignores tuple_lexicographic_chains, so create_field_ref() materializes explicit bounds for tuple-chain predicates. Please strip LowCardinality here (and in the sparse branch below) to match the scalar path.

@clickhouse-gh

clickhouse-gh Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.50% 86.50% +0.00%
Functions 91.90% 91.90% +0.00%
Branches 78.70% 78.70% +0.00%

Changed lines: Changed C/C++ lines covered: 259/283 (91.52%) · 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

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants