Skip to content

Support index pruning for IS NOT DISTINCT FROM and IS TRUE#110006

Merged
alexey-milovidov merged 25 commits into
ClickHouse:masterfrom
groeneai:fix-key-condition-is-not-distinct-from-109995
Jul 20, 2026
Merged

Support index pruning for IS NOT DISTINCT FROM and IS TRUE#110006
alexey-milovidov merged 25 commits into
ClickHouse:masterfrom
groeneai:fix-key-condition-is-not-distinct-from-109995

Conversation

@groeneai

@groeneai groeneai commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Changelog category (leave one):

  • Improvement

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

IS NOT DISTINCT FROM and IS TRUE now use primary-key and minmax indexes to prune granules, the same as =. Previously k IS NOT DISTINCT FROM 42 and (k = 42) IS TRUE scanned all granules.

Description

Closes #109995.

KeyCondition::atom_map did not map isNotDistinctFrom (the <=> operator that IS NOT DISTINCT FROM lowers to), so k IS NOT DISTINCT FROM 42 fell through to Condition: true and scanned all granules. For a non-NULL constant, key <=> c matches the same rows as key = c for both Nullable and non-Nullable keys, so it is mapped to the same FUNCTION_IN_RANGE atom as equals. NULL constants are rejected before the atom is built, because key <=> NULL means "key IS NULL", not "key = NULL".

(k = 42) IS TRUE is lowered by the analyzer to isNotDistinctFrom(equals(k, 42), true), where the key-side argument is a boolean sub-expression, not the key column. tryRewriteIsTrueCondition unwraps isNotDistinctFrom(X, true) to X in truth-tested boolean position when X is a boolean-returning function, so the inner predicate becomes a prunable key atom. The rewrite is gated to boolean-result functions because for a non-boolean X, X <=> true means X = 1, not "X is truthy".

Version info

  • Merged into: 26.7.1.1199 (included in 26.7 and later)
  • Backported to: 26.6.2.100

KeyCondition::atom_map did not map isNotDistinctFrom (the <=> operator that
IS NOT DISTINCT FROM lowers to), so `k IS NOT DISTINCT FROM 42` fell through
to Condition: true and scanned all granules. For a non-NULL constant, key <=> c
matches the same rows as key = c for both Nullable and non-Nullable keys, so it
is mapped to the same FUNCTION_IN_RANGE atom as equals. NULL constants are
rejected before the atom is built, because key <=> NULL means "key IS NULL",
not "key = NULL".

(k = 42) IS TRUE is lowered by the analyzer to isNotDistinctFrom(equals(k, 42),
true), where the key-side argument is a boolean sub-expression, not the key
column. tryRewriteIsTrueCondition unwraps isNotDistinctFrom(X, true) to X in
truth-tested boolean position when X is a boolean-returning function, so the
inner predicate becomes a prunable key atom. The rewrite is gated to
boolean-result functions because for a non-boolean X, X <=> true means X = 1,
not "X is truthy".

Closes: ClickHouse#109995

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? Yes. EXPLAIN indexes=1 SELECT count() FROM t WHERE k IS NOT DISTINCT FROM 42 shows Granules: 12/12 on master vs Granules: 1/12 with the fix (100k rows, ORDER BY k, granularity 8192). Same for (k = 42) IS TRUE and for a minmax skip index (98/98 -> 1/98).
b Root cause explained? atom_map never mapped isNotDistinctFrom (the <=> operator IS NOT DISTINCT FROM lowers to), so the atom fell through to Condition: true. (k=c) IS TRUE lowers to isNotDistinctFrom(equals(k,c), true), whose key-side arg is a boolean sub-expression, not the key column, so even the atom fix does not reach it.
c Fix matches root cause? Yes. (1) atom_map maps isNotDistinctFrom to the same FUNCTION_IN_RANGE as equals; (2) reverseComparisonOperator handles the symmetric func(const, key) order; (3) the equals/notEquals deterministic-wrap fallback includes it; (4) tryRewriteIsTrueCondition unwraps isNotDistinctFrom(X, true) -> X in boolean position for the IS TRUE case.
d Test intent preserved / new tests added? New test 04401_key_condition_is_not_distinct_from. No existing test weakened.
e Demonstrated both directions? Yes: fails (12/12, no pruning) without the fix, passes (1/12) with it; new test asserts pruning + a full correctness matrix.
f Fix general, not a narrow patch? Covers PK + minmax skip index, Nullable + non-Nullable keys, both arg orders, and ordered forms ((k < c) IS TRUE). All funnel through isNotDistinctFrom, so one fix covers IS NOT DISTINCT FROM and IS TRUE.
g Generalizes across inputs (wrappers, params, boundaries)? NULL constant rejected (<=> NULL = IS NULL, not = NULL); NaN constant declines analysis (existing path). IS TRUE unwrap gated to boolean-result functions, so non-boolean X <=> true (= X = 1) is not misrewritten; IS FALSE (<=> false) is not touched. Verified on Nullable/non-Nullable keys and (k+1) IS TRUE/k IS TRUE correctness.
h Backward compatible? Yes. Pure query-optimization addition (missed pruning). Results unchanged; only fewer granules scanned. No setting/format/default change.
i Invariants and contracts preserved? The new atom uses the exact same Range(value) semantics as equals. The IS TRUE unwrap only fires in non-inverted boolean (truth-tested) position, matching the existing tryRewriteCoalesceCondition contract where false and NULL both reject the row. Correctness verified across the full matrix.

Session id: cron:clickhouse-worker-slot-3:20260710-140500

@groeneai

Copy link
Copy Markdown
Contributor Author

cc @CurtizJ @shankar-iyer for review: this extends KeyCondition to prune primary-key and minmax indexes for IS NOT DISTINCT FROM and IS TRUE, mapping <=> (non-NULL const) to the same range atom as = and unwrapping isNotDistinctFrom(X, true) in boolean context.

@clickhouse-gh

clickhouse-gh Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [8edfda4]

Summary:


AI Review

Summary

This PR extends KeyCondition pruning for IS NOT DISTINCT FROM, IS TRUE, the added positive-boolean wrappers, and the related NULL / wrapper-safety cases, with broad stateless coverage in 04304 and 04401. I did not find a remaining correctness, safety, or contract issue in the current head after rechecking the final KeyCondition.cpp state against the earlier review threads, so there are no new inline findings to post.

Missing context / blind spots
  • ⚠️ I could not execute the new queries locally because this checkout does not include a built ClickHouse binary. The review is based on the current source, the added regression tests, and the recorded PR discussion / CI notes.
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jul 10, 2026
@PedroTadim

PedroTadim commented Jul 10, 2026

Copy link
Copy Markdown
Member

@groeneai also check partition pruning in this test case: https://fiddle.clickhouse.com/c6e6ab80-3c5b-4146-8146-ad256fa5fb1d

The isNotDistinctFrom KeyCondition mapping added in this PR now prunes
the granules for the boolean-key IS TRUE / IS FALSE predicates in
04304_primary_key_is_true_false_unknown (SelectedMarks: IS FALSE 3->1,
IS TRUE 3->2). Results are unchanged; only granule reads drop, which is
the intended optimization. Update the reference and the explanatory
comment accordingly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread src/Storages/MergeTree/KeyCondition.cpp Outdated
tryRewriteIsTrueCondition gated on the raw child of isNotDistinctFrom(X, true)
being a boolean-result function. Equivalent wrapped forms like
CAST(k = 42, 'UInt8') IS TRUE or materialize(k = 42) IS TRUE reached the gate
as CAST / materialize (not boolean-result functions) and returned nullptr, so
the wrapped predicate did not become a prunable key atom even though the later
clone strips those wrappers anyway. Peel alias / materialize / trivial CAST
(the same wrappers cloneDAGWithInversionPushDown strips) before the gate so
these equivalent predicates prune identically.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Fixed in cb61eef. tryRewriteIsTrueCondition now peels the same non-semantic wrappers cloneDAGWithInversionPushDown strips transparently (alias, materialize, trivial CAST) before the boolean_result_functions gate, so CAST(k = 42, 'UInt8') IS TRUE and materialize(k = 42) IS TRUE now prune to the single matching granule instead of falling back to Condition: true.

Verified on a UInt32 key (index_granularity = 8, 80 rows, 10 granules):

Predicate Before After
(k = 42) IS TRUE 1/10 1/10
CAST(k = 42, 'UInt8') IS TRUE 10/10 1/10
materialize(k = 42) IS TRUE 10/10 1/10

Result counts are unchanged (1 row each). The gate checks the boolean-result-ness of the peeled node but still clones the original predicate, so the recursion strips the wrappers consistently under boolean context. Regression coverage added to 04304_primary_key_is_true_false_unknown (EXPLAIN indexes = 1 granule-count assertions for the wrapped forms).

Note: a non-trivial CAST over a Nullable predicate (e.g. CAST(b = true, 'UInt8') on Nullable(Bool)) is intentionally not peeled - it is not a trivial cast and throws on NULL, so it correctly does not match.

@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (bot follow-up: wrapper peeling)
# Question Answer
a Deterministic repro? Yes. UInt32 key, index_granularity = 8, 80 rows: EXPLAIN indexes = 1 SELECT count() FROM t WHERE CAST(k = 42, 'UInt8') IS TRUE shows Granules: 10/10 before the fix.
b Root cause explained? tryRewriteIsTrueCondition checked boolean_result_functions on the raw child of isNotDistinctFrom(X, true). When X is CAST(...)/materialize(...)/alias the name is not in the set, so it returned nullptr; the later clone stripped the wrapper anyway, leaving isNotDistinctFrom(equals(k,42), true) where extractAtomFromTree falls back to Condition: true.
c Fix matches root cause? Yes. Peel the same wrappers cloneDAGWithInversionPushDown strips (alias, materialize, trivial CAST) before the boolean-result gate; still clone the original predicate so the recursion strips them consistently.
d Test intent preserved / new tests added? Yes. Existing 04304 assertions unchanged; added EXPLAIN indexes = 1 granule-count assertions for the wrapped forms plus count checks.
e Both directions demonstrated? Yes. CAST/materialize wrappers: 10/10 before -> 1/10 after (Build-ID-verified binaries, reverted-hunk A/B).
f Fix is general across code paths? Yes. Peels all three wrappers cloneDAGWithInversionPushDown handles; uses the shared isTrivialCast helper so CAST/_CAST are treated identically.
g Fix generalizes across inputs? Yes. Verified plain-equals, bool key, Nullable(Bool) trivial cast; a non-trivial Nullable->UInt8 cast is correctly NOT peeled (throws on NULL, must not prune).
h Backward compatible? Yes. Pure key-analysis improvement; no setting/format/default change. Only widens pruning for already-equivalent predicates.
i Invariants and contracts preserved? Yes. Boolean-result gate still applies to the peeled node; the rewrite stays restricted to non-inverted boolean position; result counts unchanged across all forms.

Session id: cron:clickhouse-worker-slot-4:20260710-162000

Comment thread src/Storages/MergeTree/KeyCondition.cpp Outdated
Comment thread src/Storages/MergeTree/KeyCondition.cpp
Partition pruning goes through a separate KeyCondition path (PartitionPruner
over the partition value / minmax) than primary-key granule pruning. The
isNotDistinctFrom atom map and IS TRUE rewrite already apply there, so
b IS TRUE and b IS NOT DISTINCT FROM true now prune partitions the same as
b = true. Add EXPLAIN indexes=1 Parts-count assertions plus correctness checks.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Partition pruning is covered by this PR too.

The fiddle scenario reproduced on the patched binary (PARTITION BY b, ORDER BY id):

Predicate Public 26.7 (before) This PR (after)
b = true Partition 1/1, Min-Max 1/2 Partition 1/1, Min-Max 1/2
b IS TRUE Partition 2/2, Min-Max 2/2 Partition 1/1, Min-Max 1/2
b IS NOT DISTINCT FROM true (same, no pruning) Partition 1/1, Min-Max 1/2

PartitionPruner builds a KeyCondition over the partition key expression, so it goes through the same atom_map / tryRewriteIsTrueCondition code this PR extends. No separate change was needed; the same rewrite that prunes primary-key granules also prunes the b IS TRUE / b IS NOT DISTINCT FROM true partitions down to 1/2 parts.

Added regression coverage to 04401_key_condition_is_not_distinct_from (EXPLAIN indexes = 1 Parts: 1/2 assertions for the PARTITION BY b table plus correctness checks). Pushed as 2442524.

…true)

Follow-up to the IS TRUE / IS NOT DISTINCT FROM rewrite. The same
KeyCondition rewrite now also peels two more truth-equivalent positive
boolean wrappers so a wrapped predicate becomes a prunable key atom:

  - notEquals(X, false)  i.e.  (k = 42) != false
  - in(X, <all-true const set>)  i.e.  (k = 42) IN (true)

Both are truth-equivalent to bare X only when X is boolean-valued, so
the same boolean-result gate as the IS TRUE path applies (extracted into
predicateIsBooleanResult). The IN path only handles literal/constant
sets whose explicit elements are available and every element is exactly
true (numeric 1); it declines otherwise (X IN (false), X IN (true,
false), X IN (2), empty set, subquery set), falling back to existing
behavior.

Benefits every index that goes through this shared canonicalizer:
primary key, minmax, bloom_filter and partition pruning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Extended the same KeyCondition rewrite to two more truth-equivalent positive boolean wrappers (d0bfa20), so they prune identically to the bare atom across primary key, minmax, bloom_filter and partition pruning:

  • notEquals(X, false)(k = 42) != false
  • in(X, <all-true const set>)(k = 42) IN (true) / IN (true, true) / IN (1)

Refactored the boolean-result gate into predicateIsBooleanResult (shared by both tryRewriteIsTrueCondition and the new tryRewriteInTruthyCondition). The != false case is folded into tryRewriteIsTrueCondition (const 0 instead of 1). The IN case only fires for literal/constant sets whose explicit elements are available and every element is exactly true (numeric 1); it declines (and falls back to existing behavior) for IN (false), IN (true, false), IN (2), the empty set, NOT IN, and not-yet-built subquery sets.

Regression coverage added to 04401_key_condition_is_not_distinct_from (EXPLAIN indexes=1 pruning assertions + correctness) for all four index families and the negative cases.

Pre-PR validation gate (follow-up: != false and IN (only-true))
# Question Answer
a Deterministic repro? Yes. EXPLAIN indexes=1 SELECT count() FROM pk WHERE (k=42) != false / ... IN (true) show Condition: (k in [42,42]), Granules: 1/N where the un-peeled form showed Condition: true, full scan.
b Root cause explained? notEquals(X,false) and in(X,{true}) hit no atom_map entry, so extractAtomFromTree fell back to Condition: true (no pruning). For boolean-valued X both are truth-equivalent to bare X; peeling them at the shared cloneDAGWithInversionPushDown layer exposes the inner key atom.
c Fix matches root cause? Yes. Peels the wrapper to bare X at the same shared layer as the existing IS TRUE rewrite, gated by the same boolean-result check.
d Test intent preserved / new tests added? New EXPLAIN + correctness assertions in 04401; existing assertions unchanged. Negatives (IN (false), IN (true,false), IN (2), != true, NOT IN (true)) asserted to NOT prune.
e Demonstrated both directions? Yes. Peeled forms prune to Granules: 1/N / Parts: 1/2; non-equivalent forms stay N/N / full parts. Verified on debug binary.
f Fix general, not narrow patch? Single shared canonicalizer layer -> benefits PK, minmax, bloom_filter and partition pruning. Both argument orders handled (symmetric ops).
g Generalizes across inputs? IN fires only for all-true const sets; declines empty/false/mixed/non-1/NULL element and subquery-not-built sets. Non-boolean X (e.g. k IN (true) = k=1) excluded by the boolean-result gate.
h Backward compatible? Yes. Pure pruning improvement; results unchanged (verified). No setting/format change.
i Invariants preserved? Only rewrites in non-inverted boolean context; clones the original predicate so the recursion strips the same wrappers. Set is built via buildOrderedSetInplace + hasExplicitSetElements (the same guard tryPrepareSetIndexForIn uses).

Session id: cron:clickhouse-worker-slot-15:20260710-210200

groeneai and others added 3 commits July 10, 2026 22:44
Build (arm_tidy) failed with cppcoreguidelines-init-variables:
'variable expected_const is not initialized'. Every non-return path
assigns it, so the initializer is defensive/dead, but tidy runs as
WERROR. Initialize to 0.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tryRewriteInTruthyCondition called buildOrderedSetInplace unconditionally
while cloning the key-condition DAG, which runs for every query on the
primary-key path. For X IN (SELECT ...) this executed the subquery purely
for analysis, so EXPLAIN SELECT ... WHERE c IN (SELECT throwIf(1)) with
use_skip_indexes=0 threw (server abort in debug/sanitizer builds) even
though no index is used, breaking 02707_skip_index_with_in.

Only inspect an already-built set (FutureSet::get() non-null for literal
tuple and storage sets); decline a not-yet-run subquery set. This matches
the intent that the IN set is not built when indexes are not used.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…onotonic chain

applyFunction (the range path) passed a raw ColumnLowCardinality to
func->execute while the monotonic-function chain is built against the
outer-LowCardinality-stripped key type, so a specialized UInt8->Bool CAST
wrapper did checkAndGetColumn<ColumnUInt8> on a ColumnLowCardinality and
aborted with a bad cast. Strip outer LowCardinality from the argument
column and type (and keep the cached result full), matching the sibling
applyFunctionChainToColumn.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Pushed d05c97b to fix an AST-fuzzer finding on this PR (AST fuzzer (amd_debug, targeted, old_compatibility), STID 1499-58ee): Bad cast from DB::ColumnLowCardinality to DB::ColumnVector<char8_t> in KeyCondition::applyMonotonicFunctionsChainToRange -> applyFunction -> createUInt8ToBoolWrapper.

Root cause is pre-existing on master, not introduced by this PR: applyFunction (the range path) fed a raw ColumnLowCardinality to func->execute while the monotonic chain is built against the outer-LowCardinality-stripped key type, so the specialized UInt8->Bool CAST wrapper did checkAndGetColumn<ColumnUInt8> on a ColumnLowCardinality and aborted. Confirmed by A/B on Build-ID-verified binaries: the minimal SELECT count() FROM t WHERE toLowCardinality((SELECT false)) = b (plain equals atom, no IS TRUE) aborts on the PR base commit and succeeds after the fix. This PR's new LowCardinality(Bool) PK test table just gave the fuzzer material to reach it (CIDB: hit unrelated PRs #106430, #106538 before). Fix strips outer LowCardinality from the argument column/type before executing, matching the sibling applyFunctionChainToColumn.

Regression coverage added to 04401_key_condition_is_not_distinct_from (LC(Bool) key vs LC constant). Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110006&sha=6278f1dba7154a0f2b48014e5c3d2f7c6e626305&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%2C%20targeted%2C%20old_compatibility%29

…table

The lc_bool regression table sets index_granularity_bytes = 0 (non-adaptive
granularity), which is incompatible with the default min_bytes_for_wide_part
(10 MB Compact threshold). MergeTree then logs a Warning to stderr, and Fast
test fails any test that writes to stderr. Add min_bytes_for_wide_part = 0 to
force Wide format, matching the non-adaptive granularity. Test output is
unchanged (reference already matched); this only silences the warning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

CI finish ledger — 39b6ace

Every failure below has an owner (a fixing PR). Only CH Inc sync is exempt.

Check / test Reason Owner / fixing PR
Integration tests (amd_tsan, 3/6) / test_replicated_database::test_replicated_table_structure_alter flaky (30d: 15+ unrelated PRs + master) #110030 (ours, open)
Stress test (amd_tsan) / Logical error 'clock' (STID 2508-4031) trunk fuzzer bug in EXPLAIN ANALYZE step-clock (feature PR #106586); 0 master, not PR-caused #110077 (ours, open)
Sync CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260711-053000

…L to isNull

Address two clickhouse-gh[bot] review Majors on the positive-boolean-wrapper
key pruning:

- Derive the boolean-result gate in predicateIsBooleanResult from
  KeyCondition::atom_map (single source of truth) instead of a hand-maintained
  comparison list, so IS TRUE / != false / IN (true) wrappers peel to ANY
  prunable boolean atom KeyCondition supports (has, startsWith, startsWithUTF8,
  match, pointInPolygon, ...), not just comparisons. startsWith(s,'ab') IS TRUE
  now prunes the key the same as bare startsWith(s,'ab').

- Route the NULL arm of isNotDistinctFrom onto the existing isNull atom:
  key <=> NULL is equivalent to isNull(key), so a Nullable PK / minmax index
  now prunes to the NULL granule exactly instead of scanning every granule.

Add 04401 coverage for the newly-covered forms and flip the previously-weaker
IS NOT DISTINCT FROM NULL assertion to assert pruning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Both addressed in 6be0f9e.

Major 1 (IS TRUE gate too narrow). predicateIsBooleanResult now derives the allowlist from KeyCondition::atom_map (the single source of truth for prunable atoms) plus a small extra_boolean_result_functions set for the boolean-valued functions that are not atoms (not, and, or, isDistinctFrom, ilike, notILike). So the wrapper peels to any atom KeyCondition can prune, including has, startsWith, startsWithUTF8, match, pointInPolygon. Verified on a String PK (Granules: 12/12 -> 1/12):

startsWith(s, '999') IS TRUE   -- prunes like bare startsWith(s, '999')
startsWith(s, '999') != false  -- same
startsWith(s, '999') IN (true) -- same

Major 2 (NULL arm of IS NOT DISTINCT FROM). The const IS NULL branch for isNotDistinctFrom no longer bails out. key <=> NULL is equivalent to isNull(key), so it now reuses the existing isNull atom and prunes a Nullable PK / minmax index to the NULL granule exactly (both key <=> NULL and NULL <=> key), matching bare key IS NULL (Granules: 13/13 -> 1/13). The 04401 assertion that previously codified the weaker no-prune behavior is flipped to assert the pruning.

Both directions A/B verified on Build-ID-matched binaries. 04401 extended (.sql+.reference); 04304 / 03773 / 02707 still pass.

@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand) — commit 6be0f9e
# Question Answer
a Deterministic repro? Yes. EXPLAIN indexes=1 ... WHERE startsWith(s,'999') IS TRUE (Major 1) and ... WHERE nullable_key IS NOT DISTINCT FROM NULL (Major 2) show non-pruning on demand pre-fix.
b Root cause explained? Major 1: the boolean-result gate was a hand-maintained comparison list, so non-comparison prunable atoms (has/startsWith/match/pointInPolygon) failed the gate and the isNotDistinctFrom(X,true) wrapper was left un-prunable. Major 2: the isNotDistinctFrom NULL-const arm returned false (declined), leaving key <=> NULL unanalyzed so the Nullable index scanned all granules.
c Fix matches root cause? Yes. Gate derived from atom_map (single source of truth) + extras for non-atom boolean funcs; NULL arm routed onto the existing isNull atom.
d Test intent preserved / new tests added? Yes. The prior IS NOT DISTINCT FROM NULL no-prune assertion (which codified the weaker behavior the bot flagged) is flipped to assert pruning; added startsWith IS TRUE/!=false/IN(true) + const-on-left NULL coverage. No assertion weakened.
e Demonstrated both directions? Yes. A/B on Build-ID-matched binaries: startsWith IS TRUE 12/12->1/12; <=> NULL 13/13->1/13. Pre-fix binary 8D27ED9D, post-fix DA5702F6.
f Fix general, not a narrow patch? Yes. Deriving from atom_map covers ALL current and future prunable atoms in one place rather than patching one function name; the NULL arm handles both operand orders (key <=> NULL and NULL <=> key).
g Generalizes across inputs / wrappers? Yes. Verified across wrapped forms (CAST, materialize, alias) still peel; LowCardinality(Bool) key path (04401 lc_bool) unaffected; comparison / in / minmax / partition-pruning atoms unchanged (04304 IS TRUE cases still prune to 1 granule).
h Backward compatible? Yes. No setting default / format / metadata change. Both changes strictly ENABLE additional pruning that yields identical query results (correctness checks unchanged).
i Invariants / contracts preserved? Yes. Major 2 mirrors the existing unary isNull atom path exactly (same isKeyPossiblyWrappedByMonotonicFunctions + key_column_num init check + out population), so the RPNElement contract for FUNCTION_IS_NULL holds. Major 1 only widens which predicates reach the already-correct clone-and-peel path under non-inverted boolean context.

Session id: cron:clickhouse-worker-slot-1:20260711-054900

Comment thread src/Storages/MergeTree/KeyCondition.cpp
ifNull(k = 42, 0) IS TRUE (and the != false / IN (true) forms) lowered to
isNotDistinctFrom(ifNull(equals(k, 42), 0), true) did not prune: the
boolean-result gate in predicateIsBooleanResult rejects the ifNull wrapper, so
tryRewriteIsTrueCondition declines and the fallback clone with boolean_context
= false denies the existing coalesce rewrite its chance too, leaving
Condition: true even though bare ifNull(k = 42, 0) already prunes when
allow_key_condition_coalesce_rewrite is enabled.

Recurse the boolean-result gate through a falsy-zero ifNull/coalesce wrapper
(gated by the same setting): ifNull(Y, 0) is boolean-valued iff Y is, so the
outer peel enters boolean context and the existing recursion unwraps the ifNull
to the prunable inner atom. Factor the ifNull/coalesce(X, 0) shape recognition
into isFalsyZeroCoalesceCondition, shared by the gate and tryRewriteCoalesceCondition.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread src/Storages/MergeTree/KeyCondition.cpp Outdated
functionIsTotalOnKeyDomain used canBeSafelyCast + is_always_monotonic to
decide totality. That misses intDiv(k, -1): getMonotonicityForRange reports
is_always_monotonic for a -1 divisor, yet intDiv(Int_MIN, -1) overflows and
raises ILLEGAL_DIVISION for key widths without the constant-divisor
specialization (Int8/Int16/Int128). So isNull(intDiv(k, -1)) could reuse the
bare isNull atom and prune the non-NULL granule, suppressing the exception.

Replace the heuristics with a behavioral boundary probe: run the wrapper on the
extreme values of its integer key domain via the same execute path a scan uses,
and decline the isNull-atom reuse if it raises. This catches conversion
overflow, div-by-zero and the signed min/-1 case uniformly, and admits the
Int32/Int64 intDiv(k, -1) forms that wrap (never raise). Non-integer domains
fall back to the conservative type-level canBeSafelyCast check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? Yes. isNull(intDiv(k, -1)) on a Nullable(Int8) MergeTree key with an Int8_MIN row: baseline reused the isNull atom, pruned to Granules: 1/2 and returned the NULL row instead of raising on scan. EXPLAIN indexes=1 reproduces the wrong prune deterministically.
b Root cause explained? functionIsTotalOnKeyDomain decided totality from canBeSafelyCast (types) + is_always_monotonic. getMonotonicityForRange reports is_always_monotonic for an intDiv(k, -1) divisor, but intDiv(Int_MIN, -1) overflows and raises ILLEGAL_DIVISION for key widths with no constant-divisor specialization (Int8/Int16/Int128). The FUNCTION_IS_NULL evaluators ignore the monotonic chain, so reusing the atom pruned the non-NULL granule and suppressed that exception.
c Fix matches root cause? Yes. Replaced the type-level + monotonicity heuristics with a behavioral boundary probe: run the wrapper on the extreme values of its integer key domain via the same execute path a scan uses, and decline if it raises. This is exactly the "probe that boundary" the review asked for.
d Test intent preserved / new tests added? Preserved. Added 04401 EXPLAIN indexes=1 no-prune assertions for the Int8 intDiv(k, -1) forms, a prune assertion for the total Int64 form, and a correctness count.
e Demonstrated in both directions? Yes, on Build-ID-matched binaries: baseline pruned Int8 isNull(intDiv(k, -1)) to 1/2; fixed keeps 2/2 (Condition: true), and the total Int32/Int64 and intDiv(k, 2) / toUInt32 forms still prune 1/2.
f Fix is general, not a narrow patch? Yes. The single probe subsumes all three partiality classes (conversion overflow toDateTime(Date32) throw mode, div-by-zero, signed min / -1) across every native integer width, rather than name- or type-matching one case.
g Generalizes across inputs / datatypes / wrappers? Yes. Probe covers {UInt,Int}{8,16,32,64,128,256} min/max/-1/0/1; verified intDiv(k, -1) declines on Int8/Int16/Int128 and stays total on Int32/Int64 (wrap), intDiv(k, 0) declines all widths, toDateTime(Date32) throw declines, toUInt32(Nullable) / intDiv(k, 2) / widening casts still prune. Nullable/LowCardinality stripped before probing.
h Backward compatible? Yes. Pure index-analysis soundness fix; declining only forces a full scan, no format/setting/default change.
i Invariants and contracts preserved? Yes. The isNull-atom reuse now holds the invariant it silently assumed (wrapper total on the key domain). Probe is read-only, catches exceptions, and never mutates state; on a non-integer domain it falls back to the prior conservative canBeSafelyCast check.

Session id: cron:clickhouse-worker-slot-4:20260712-121200

Comment thread src/Storages/MergeTree/KeyCondition.cpp Outdated
if (!key_arg_type || !isNullableOrLowCardinalityNullable(key_arg_type))
return true;

try

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These two helpers only gate when isNull / IS NOT DISTINCT FROM NULL over a monotonic-wrapped key may reuse the bare isNull atom. I can drop the runtime probing entirely and reuse that atom only for a bare Nullable key (empty function chain), falling back to a full scan whenever the key is wrapped by any monotonic function. Core IS TRUE / IS NOT DISTINCT FROM <non-null> pruning is unaffected. Want me to go that way, or would you prefer the wrapped-key null atom handled differently?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 3d34f3c. Removed both runtime-probing helpers. The isNull atom is now reused only for a bare Nullable key (empty monotonic-functions chain); any wrapper declines and full-scans, which is always correct. This also subsumes every wrapper class the earlier rounds chased (NULL-erasing, overflow-checked conversions, intDiv). Core IS TRUE / IS NOT DISTINCT FROM pruning is unchanged.

Comment thread src/Storages/MergeTree/KeyCondition.cpp Outdated
auto column = from_type->createColumn();
for (const auto & field : *extremes)
column->insert(field);
try

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed in 3d34f3c along with the NULL-preservation probe; the totality machinery is gone. A wrapped key no longer reuses the isNull atom at all.

The isNull/isNotNull atoms (and the key IS NOT DISTINCT FROM NULL branch that
reuses them) ignore the key monotonic-functions chain, so they are sound only
for a bare Nullable key. A monotonic wrapper can erase NULL (ifNull, coalesce,
CAST to non-Nullable) or be partial (toDateTime(Date32) / intDiv(k, 0) throw on
some rows), which would let isNull(wrapper(k)) narrow to the NULL granule and
mark it exact-true, returning wrong exact-count results. Decline the atom for
any wrapped key and fall back to a full scan, which is always correct.

This removes the runtime function-probing helpers (functionPreservesNulls,
functionIsTotalOnKeyDomain) in favor of a plain empty-chain check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand)
# Check Result
a Deterministic repro? N/A (design simplification requested by reviewer, not a new bug). The prior rounds' repros (ifNull/CAST NULL-erasing, toDateTime(Date32) throw, intDiv(k,0)) all still resolve correctly: a wrapped key now declines the isNull atom and full-scans.
b Root cause explained? The isNull/isNotNull atoms (and the key IS NOT DISTINCT FROM NULL branch) ignore the monotonic-functions chain, so reusing them over a wrapped key is unsound in several independent ways (NULL erasure, partiality). Deciding safety per-wrapper required executing functions during analysis.
c Fix matches root cause? Yes. Reuse the atom only for a bare key (empty chain); decline any wrapper and full-scan. Removes the need to reason about individual wrappers at all.
d Test intent preserved / new tests? Yes. 04401 keeps the correctness + EXPLAIN assertions; the wrapped-key EXPLAIN cases now assert no-prune uniformly, and all correctness counts are unchanged (a full scan returns the same rows). serverError assertions for partial wrappers still hold (scan raises).
e Demonstrated both directions? Yes. Correctness counts / serverError markers unchanged; EXPLAIN no-prune verified for every wrapped form; bare k IS NULL / k <=> NULL still prune.
f Fix general, not a narrow patch? Yes, strictly more general: a single empty-chain gate covers every wrapper family instead of enumerating them.
g Generalizes across inputs? Yes. The gate is wrapper-agnostic (any non-empty chain declines), so it covers all type wrappers, conversions, and arithmetic uniformly.
h Backward compatible? Yes. No setting/format change. This only narrows an optimization that shipped within this same PR; declining always yields a correct full scan.
i Invariants preserved? Yes. The FUNCTION_IS_NULL evaluators' assumption (chain is ignored) is now only reached for a bare key, restoring the invariant they rely on.

Session id: cron:clickhouse-worker-slot-0:20260712-165800

Comment thread tests/queries/0_stateless/04304_primary_key_is_true_false_unknown.sql Outdated
The 04304 EXPLAIN/SelectedMarks assertions hard-code granule counts, but
bool_pk and int_pk left index_granularity_bytes randomizable, so a small
randomized adaptive-granularity value could split the tables into a
different number of marks and break the assertions. Pin
index_granularity_bytes = 0 and min_bytes_for_wide_part = 0 on both tables,
matching 04401_date32_zerotransform_monotonicity_pruning in this PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand)
# Check Result
a Deterministic repro? Yes. With randomized index_granularity_bytes the 24-row bool_pk / 80-row int_pk tables can split into a different mark count, so the hard-coded SelectedMarks / Granules: N/10 assertions become flaky.
b Root cause explained? The tests assert exact granule counts but left adaptive granularity (index_granularity_bytes) randomizable. A small randomized value re-splits the table into more marks, breaking the counts even though pruning is still correct.
c Fix matches root cause? Yes. Pin index_granularity_bytes = 0 (fixed-row granularity) and min_bytes_for_wide_part = 0 on both tables, so the layout is exactly index_granularity = 8 regardless of randomization.
d Test intent preserved? Yes. The pruning assertions are unchanged; only the layout is pinned so they measure the primary key alone. Same precedent already used by 04401_date32_zerotransform_monotonicity_pruning in this PR.
e Both directions? N/A (test-only stability fix, no product behavior change). Verified the pinned layout produces exactly the reference: bool_pk = 1 part / 3 granules, IS TRUE 2/3, IS FALSE 1/3, IS UNKNOWN 2/3, IS NOT UNKNOWN 2/3; int_pk block matches the reference byte-for-byte.
f Fix general, not narrow patch? N/A (test setting pin, not a code bug). Applied to both tables in the file that hard-code granule counts.
g Generalizes across inputs? N/A (test setting pin).
h Backward compatible? N/A (test-only).
i Invariants preserved? N/A (test-only).

Verified via clickhouse-local: full bool_pk count block + entire int_pk block match the reference; bool_pk granule pruning confirmed via EXPLAIN indexes = 1. The system.query_log SelectedMarks readout runs only on a full server (present in CI, absent on the stripped local runner).

Session id: cron:clickhouse-worker-slot-3:20260712-181200

@groeneai

Copy link
Copy Markdown
Contributor Author

CI finish ledger — d551f23

Every failure below has an owner (a fixing PR). No failure is PR-caused; the PR's KeyCondition/04304/04401 changes are unrelated to the two flaky checks that failed.

Check / test Reason Owner / fixing PR
Stateless tests (amd_tsan, s3 storage, parallel, 2/3) / 01730_distributed_group_by_no_merge_order_by_long flaky (Code 241 memory limit; 4/4 reruns passed, 47 unrelated PRs in 30d) #109847 (external, open)
Stateless tests (amd_tsan, s3 storage, parallel, 2/3) / Scraping system tables infra (minio_audit_logs dump timeout on s3 configs; 37 unrelated PRs in 30d) #110099 (ours, open)
CH Inc sync - CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260712-230000

@clickhouse-gh

clickhouse-gh Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.80% 85.80% +0.00%
Functions 92.70% 92.70% +0.00%
Branches 78.00% 78.00% +0.00%

Changed lines: Changed C/C++ lines covered: 139/155 (89.68%) · Uncovered code

Full report · Diff report

@alexey-milovidov alexey-milovidov self-assigned this Jul 20, 2026
@alexey-milovidov
alexey-milovidov merged commit 49b3041 into ClickHouse:master Jul 20, 2026
175 of 176 checks passed
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 20, 2026
groeneai added a commit to groeneai/ClickHouse that referenced this pull request Jul 20, 2026
…ey-over-merge

Reconcile the KeyCondition::applyFunction cache-miss LowCardinality strip
with the sibling fix from ClickHouse#110006 (merged): keep the recursive strip
(covers nested Array(LowCardinality(T))) for the input column and type,
and keep ClickHouse#110006's result hardening (removeLowCardinality on the result
type + convertToFullColumnIfLowCardinality on the result column).
applyFunctionForField and the sparse checkInHyperrectangle caller retain
their recursive strips.
@robot-ch-test-poll robot-ch-test-poll added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 22, 2026
robot-clickhouse-ci-1 added a commit that referenced this pull request Jul 22, 2026
Cherry pick #110006 to 26.6: Support index pruning for IS NOT DISTINCT FROM and IS TRUE
robot-clickhouse added a commit that referenced this pull request Jul 22, 2026
@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jul 22, 2026
clickhouse-gh Bot added a commit that referenced this pull request Jul 22, 2026
Backport #110006 to 26.6: Support index pruning for IS NOT DISTINCT FROM and IS TRUE
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-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-improvement Pull request with some product improvements pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo v26.6-must-backport

Projects

None yet

6 participants