Fix text and token skip indexes over-pruning IPv6 columns - #113157
Fix text and token skip indexes over-pruning IPv6 columns#113157groeneai wants to merge 3 commits into
Conversation
The ngrambf_v1 / tokenbf_v1 / sparse_grams aggregator tokenizes the raw bytes
of the indexed column, which for an IPv6 column are its 16 stored bytes.
MergeTreeConditionBloomFilterText::traverseTreeEquals gated on the CONSTANT's
type only and then tokenized the constant's own bytes, so for
WHERE ip = '2001:db8::'
the granule held n-grams of 20010DB8000000000000000000000000 while the probe
was built from the ASCII 323030313A6462383A3A. The two encodings share no
prefix, so the bloom filter reported no match and the matching granule was
pruned: 0 rows instead of 1, on a plain SELECT with no forced index and no
unusual setting.
The query is only valid because the comparison function converts separately at
runtime (executeWithConstString). Index analysis had no counterpart; the
sibling bloom_filter index does convert and is correct on the same query.
Convert the constant into the indexed column's primitive type and re-serialize
it through that type's own column, so the probe bytes are the bytes the index
stores. A constant that cannot be represented there declines the atom, which
falls back to a full scan and a correct answer; tryConvertFieldToType is used
so index analysis cannot raise for a constant the comparison would never
evaluate. String and FixedString columns keep the previous code path exactly
via an early return, since there the constant already is the index encoding
and a constant wider than a FixedString must still be able to prune.
Affected carriers, all measured: ngrambf_v1, tokenbf_v1 and sparse_grams on
IPv6, ngrambf_v1 on LowCardinality(IPv6), and the map-key subcolumn form on a
Map(IPv6, ...) with a mapKeys index. Granules are untouched, so existing
indexes stay valid and need no rebuild.
…ndex
tryPrepareSetBloomFilter is a third arm with the shape the previous commit
repaired in traverseTreeEquals: it type-gates on the SET's element types and
then tokenizes their raw bytes, without converting them into the indexed
column's encoding.
The tuple form of IN was already safe by accident. RPNBuilderTreeNode::
tryGetPreparedSet resolves a literal tuple through PreparedSets::findTuple,
which requires equals(set->getTypes(), types) against the index data types, so
a String tuple set against an IPv6 index never matches and the atom declines.
A subquery resolves through findSubquery, which takes no types argument at all,
so a String-typed set clears the gate and builds ASCII probes against the 16
stored bytes:
SELECT count() FROM t WHERE ip IN (SELECT '2001:db8::')
returned 0 where the oracle returns 1, with Granules: 0/64.
use_index_for_in_with_subqueries defaults to true.
Reuse convertConstantToIndexDomain per set row, and decline the whole atom if
any row cannot be represented in the index domain: a partially converted set
would under-approximate membership and could prune a granule that matches. A
String or FixedString domain keeps the previous code path byte for byte, since
there the set's own bytes already are the index encoding.
Also relabel the inverted notEquals test row. Skip index conditions are built
from an ActionsDAGWithInversionPushDown, whose inverse_relations renames
notEquals to equals under a NOT, so NOT (ip != 'lit') is carried by the equals
arm rather than by the notEquals arm. The row is a real carrier and stays; its
name no longer claims to observe something it cannot.
The map-key paths of MergeTreeConditionBloomFilterText::traverseTreeEquals substitute the serialized map key for the compared value, but the constant conversion added earlier in this PR was still handed the type of the comparison's right-hand side. convertFieldToType dispatches on that type, and it treats a FixedString(16) source as the binary form of an IPv6 address. So for a mapKeys index over an IPv6-keyed map, WHERE m.`key_<addr>` = toFixedString(v, 16) decoded the key's ASCII text as an address and probed the index with the wrong bytes, dropping the matching granule and returning a wrong result. Any other key-text length hit end of input instead, declined the atom and silently lost a legitimate prune. A bare string literal happens to share the key's own type, which is why this was not visible on the spellings already covered. Give the conversion the source type of the value it actually converts. The mapValues sibling keeps the right-hand side's type because it does not substitute the value. The arrayElement branch already captured the key's type and now passes it on, although that branch is currently unreachable for an IPv6-keyed map.
Internal second-model review (click to expand)Three review rounds by an independent model that did not write the code, each Round 1 — 2 findings, both accepted and fixed ❌
Round 2 — 1 finding from my own review, which the automated pass missed ❌ The conversion hint described the compared value while the value being Round 2 also closed round 1's second finding. The implementer escalated 💡 Noted, not blocking: three open pull requests now queue on this file Round 3 — clean. I enumerated the reachable constructions independently Gate spend across all rounds: $22.25. |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-review-slot-50:20260803-152300 |
|
cc @al13n321 @rschu1ze, could you review this? Index analysis for the ngrambf_v1/tokenbf_v1/sparse_grams condition never converted the constant into the indexed column's type, so on an IPv6 column a string literal was tokenized as ASCII while the granule holds the 16 raw bytes, and the matching granule was pruned; the fix adds the conversion the bloom_filter sibling already does. |
PedroTadim
added
the
can be tested
|
Workflow [PR], commit [9f6a640] Summary: ✅
AI ReviewSummaryThis PR fixes wrong-result pruning in Final Verdict✅ No new blockers or majors. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 53/58 (91.38%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
CI finish ledger - 9f6a640No failures to own on this commit: 157 success, 18 skipped, 0 failures out of 175 check-runs. Session id: cron:our-pr-ci-monitor:20260804-000000 |
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed
ngrambf_v1,tokenbf_v1andsparse_gramsskip indexes silently dropping matching rows when anIPv6column is compared with a string literal, as inWHERE ip = '2001:db8::'. The index probe is now built from the constant converted into the indexed column's own type, so it matches the bytes the index stores.Description
WHERE ip = '2001:db8::'over anngrambf_v1/tokenbf_v1/sparse_gramsindex on anIPv6column returned 0 rows instead of 1: a plainSELECT, no forced index, no unusual setting.LowCardinality(IPv6), the map-key subcolumn form onMap(IPv6, ...), andip IN (SELECT ...)are affected the same way.Root cause. The aggregator tokenizes the indexed column's raw bytes, for
IPv6the 16 bytes20010DB800....MergeTreeConditionBloomFilterText::traverseTreeEqualschecked only the constant's type and tokenized the constant's own bytes, the ASCII2001:db8::. The encodings share no prefix, so the bloom filter reported no match and the granule was pruned. The query is valid only because the comparison function converts separately at runtime; index analysis had no conversion step. Thebloom_filterindex does convert, and is correct here.Change. Before building the probe, convert the constant into the indexed column's primitive type and re-serialize it through that type's own column, so the probe bytes are the bytes the index stores. The same conversion runs per element of an
INset, which had the same defect when the set comes from a subquery and so is not type-checked against the index. A constant that cannot be represented declines the atom, giving a full scan and a correct answer.StringandFixedStringkeep the previous path via an early return, since there the constant already is the index encoding. Granules are untouched, so existing indexes stay valid and need no rebuild.Validation. New test
04716_text_index_ipv6_string_constantuses aLogtwin as the oracle and asserts granule counts viaEXPLAIN indexes = 1, since a row count cannot see over-pruning. It covers all six carriers and keeps non-matching-address arms so pruning is not blanket-disabled. 26 assertions fail on unpatched master and pass here; 50 randomized runs are green, and a 373-test skip-index suite shows no regressions.Reported by an automated review comment on #112868.