Do not deserialize a skip index whose on-disk type is stale - #112484
Merged
alexey-milovidov merged 20 commits intoAug 15, 2026
Merged
Do not deserialize a skip index whose on-disk type is stale#112484alexey-milovidov merged 20 commits into
alexey-milovidov merged 20 commits into
Conversation
An ALTER MODIFY COLUMN type change updates the table metadata immediately and
schedules a mutation to rewrite the parts. Until that mutation runs, a part's
set/minmax skip index granules still hold bytes serialized with the OLD column
type, while index analysis builds the index sample block from the CURRENT
metadata snapshot and decodes those bytes with the NEW type.
The read gate meant to prevent that, MergeTreeDataSelectExecutor::canUseIndex,
infers the mismatch from the existence of a pending mutation ENTRY, and fails
open when that set is empty. Measured, the set is empty in four independent
ways: the mutation is pending but dropped from the storage snapshot; the
mutation was removed by KILL MUTATION; no mutation is created at all (lazy JSON
typed-path hints); or the column conversion is representation-preserving while
the index EXPRESSION's type still changes. Independently, the WHERE-clause top-k
minmax read has no canUseIndex guard at all.
The symptom depends only on the target type: a LOGICAL_ERROR in
SerializationNullable that aborts debug and sanitizer builds, a multi-exabyte
allocation request in a release build, CANNOT_READ_ALL_DATA, or, for
Int8 -> Enum8 with a value outside the new enum, a silently wrong result.
Ask the right question instead: do the part's bytes match the type they are
about to be decoded with? Mutation entries are mutable third-party state; the
part's own recorded type is not. IMergeTreeIndex::getDeserializedFormat already
receives the part and already performs a per-part schema rejection for
invalidated system columns, so the check belongs there, covering all thirteen
call sites without signature changes or scattered guards.
Since {0, {}} would then mean both "not materialized" and "materialized but not
usable", split the two questions: getPhysicalFormat (virtual) keeps the existing
checksum- and storage-aware discovery, including legacy layouts, and
getDeserializedFormat (non-virtual, so no format override can bypass the check)
performs the usability checks and delegates to it. The two existing overrides
are renamed with their discovery bodies unchanged.
removeIndexMarksFromCache moves to the physical accessor so marks cached before
the ALTER are still evicted; the prewarm loop deliberately stays on the
usability accessor, because prewarming a refused index only wastes I/O.
The type predicate is fail-closed and directional, allowing only
representation-preserving differences: identical types, Array/Nullable
recursion, enum value-set extension, and the width-preserving Enum8/Int8,
Enum16/Int16, DateTime/UInt32 and Date/UInt16 pairs. Int8 -> Enum8 is
deliberately excluded, since that direction is the wrong-result case above.
A non-trivial expression index is refused on any type difference, because
deriving the expression's part-side result type would require running the
analyzer per part on the query path. This intentionally duplicates part of
isMetadataOnlyConversion rather than calling it: that predicate answers whether
ALTER must rewrite the data, which is weaker, and reusing it fails open on JSON
type hints.
canUseIndex is left untouched. Over-firing costs pruning, not correctness.
Closes: ClickHouse#112213
isPartTypeCompatible skipped its check when the part's column list held no entry for a column the index requires, on the assumption that such a part cannot hold a granule for that index. It can: a part carries index files for an index whose required column was never written to it, which is the state MutateTask's absent-index-column handling exists to serve (issue ClickHouse#104872). A killed retype of that column then decoded the old granule under the new type through this second door, requesting exabytes. Reorder getDeserializedFormat to discover the on-disk format first and run the type check only when a format was found. The check is then only ever asked about a part that has the index, so the absent-column branch becomes an unconditional refusal and needs no metadata snapshot. A part that merely predates ADD INDEX is unaffected: it has no index files, so the physical accessor reports it as not materialized before the type check runs. Test: pin the runner-randomized top-k settings per statement, since without them the top-k assertions can run with the optimization disabled and assert nothing. Reach MergeTreeIndexReadResultPool by combining WHERE with ORDER BY and LIMIT, which the previous lines did not do, and assert the analysis-time top-k step's own description so the control proves pruning is retained rather than only that the answer is right. Tag no-parallel-replicas: the test asserts exact granule counts and sets use_skip_indexes_on_data_read.
The three statements in 04165 that set use_skip_indexes_on_data_read = 1 could never construct MergeTreeIndexReadResultPool under the stateless test profile, so they asserted only that the answer was right, which it also is without the pool. tests/config/users.d/limits.yaml sets max_rows_to_read = 20000000 in the default profile and install.sh symlinks it unconditionally, read_overflow_mode defaults to throw, and ReadFromMergeTree::supportsSkipIndexesOnDataRead() returns false on exactly that pair. Neutralize max_rows_to_read per statement, matching the in-tree idiom (03533_skip_index_on_data_reading.sql and seven other tests pair the two settings). max_rows_to_read_leaf is absent from that profile, so the symmetric guard is inert and needs no pin. Add an observable so this cannot rot back into a result-only check: when the read-time pool takes over, index analysis stops filtering and the ReadFromMergeTree summary line reports every granule, so asserting "Parts: 1 | Granules: 16" on the unaltered control fails if the pool is ever silently declined again. Measured 8/8 with the pool and 0/3 under the profile limit, and stable when prewhere, lazy materialization and bulk filtering are disabled. EXPLAIN ANALYZE is required because InterpreterExplainQuery force-disables use_skip_indexes_on_data_read for every non-ANALYZE EXPLAIN. Verified in both directions on the same binary: reverting the per-part type check makes the read-time statement fail with CANNOT_READ_ALL_DATA out of MergeTreeSelect(pool: ReadPoolInOrder) where the fix returns the unindexed answer, and removing the pins reddens only the new assertion. No source change; src is byte-identical to the previous commit.
DataTypeDateTime::equals and DataTypeDateTime64::equals deliberately ignore the
timezone, so ALTER TABLE ... MODIFY COLUMN dt DateTime('Asia/Tokyo') over a
DateTime('UTC') column is metadata-only and creates no mutation at all. The
per-part type check in IMergeTreeIndex::isPartTypeCompatible short-circuited on
that equality and never reached the expression-index refusal, so an index whose
expression reads the timezone (toHour, toStartOfDay, toDate, toYYYYMMDD, ...)
kept granules computed in the OLD zone while the query evaluates the expression
in the NEW one. The result is a wrong count: granules that genuinely match are
pruned away. Nothing downstream can notice, because the expression's result type
(UInt8 for toHour) does not change.
The equality fast path is now timezone-aware. A new file-local predicate reports
a difference when two equals-equal types attribute their values to different
timezones, walking Nullable, Array, LowCardinality, Map, Tuple and Variant
pairwise the same way those types' own equals does, so a DateTime nested in a
wrapper is reached. Two implicit-timezone types are the same type and compare
equal. The fall-through is fail-closed but scoped by reachability, so a type
that carries no timezone is never refused.
A simple single-column index is still accepted across the same ALTER: its
set/minmax granule stores the raw epoch value, which no timezone alters, and
refusing it would be a pruning regression on a very common ALTER.
DataTypeDateTime and DataTypeDateTime64 are the only types under src/DataTypes
whose equals drops a semantically load-bearing parameter; every other
implementation compares its parameters or recurses into its children. Time and
Time64 carry no per-type timezone at all, so they cannot drift.
IDataType::equals() answers "is the on-disk representation the same?". Three
implementations deliberately drop an attribute that no byte of the representation
depends on, but that a skip index EXPRESSION does read, so the stale-index check
took its equality fast path and accepted granules that no longer mean what the
query computes:
- DataTypeDateTime and DataTypeDateTime64 ignore the timezone. Round 3 handled
this for the column itself; it is also reachable through an AggregateFunction
argument, because DataTypeAggregateFunction::equals() compares argument_types
with equals() in turn. An index over finalizeAggregation(v) then keeps
granules computed in the old zone. A bare AggregateFunction column cannot be
indexed, but minmaxIndexValidator inspects the expression's RESULT type, so
finalizeAggregation() of one is accepted.
- A custom name over a plain type is invisible to the underlying
DataTypeNumber<T>::equals(), which compares only typeid. Bool is a custom
name plus a custom serialization over UInt8, so MODIFY COLUMN v Bool is
metadata-only while an index over toString(v) holds '0'/'1' and the query now
evaluates 'false'/'true'.
Both are the same defect as the timezone case, reached through a different
dropped attribute, so they are fixed by one predicate rather than a branch each:
IDataType::getName() is the one projection that carries every such attribute,
because it returns the custom name when present and each composite renders its
children's names. That replaces the per-type timezone walk with a single
comparison, and covers wrappers and nested arguments without enumerating them.
The comparison is only valid on the equals-equal path. On the NOT-equals path a
name difference is a real conversion, where isRepresentationPreservingConversion
decides and a name test would refuse the whole allow-list (Enum8('a'=1) -> Int8
and friends). It also stays scoped to non-trivial expression indexes: a granule
over the bare column holds the raw byte, which neither a timezone nor a custom
name alters, so a simple single-column index must keep pruning.
Fixing only that would still have depended on load order. The part-side type was
read through IMergeTreeDataPart::tryGetColumn(), which answers from
columns_description -- a storage-wide interning cache whose key equality is
NamesAndTypesList::operator== -> IDataType::equals(), and whose hash ignores
types entirely. Two parts whose column lists differ only by a dropped attribute
therefore share one entry, and whichever part loads first decides what both
report; part loading is concurrent and shuffled. The type now comes from the
part's own uncached list, with tryGetColumn() kept as the existence test because
it is also the only accessor that resolves a subcolumn such as j.a.
The previous round's fail-closed fallback -- "is a timezone reachable from either
side" for a shape with no pairwise branch -- over-fired in the other direction:
DataTypeObject implements forEachChild, so an UNCHANGED JSON column with a typed
DateTime path reported a difference with no ALTER at all, and every non-trivial
index over such a column lost pruning on every part forever. Comparing names
answers correctly for two identical types, so that fallback is gone.
Test cases 15-18 cover the four carriers plus an over-fire control for each. Case
15 makes the cache collision deterministic with ATTACH PARTITION FROM: the
destination interns its Asia/Tokyo column list first, then the UTC-written part
arrives and hits that entry.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit reads the part-side type from the part's own column list so the storage-wide ColumnsDescription interning cache cannot erase an attribute that IDataType::equals() drops. That covered top-level columns only; a subcolumn requirement such as p.x is not a top-level entry, so it fell back to the cached description on the argument that "a subcolumn cannot itself be a top-level column of the part, so no carrier is left on the cached path". That argument is false. The carrier is the subcolumn's PARENT, whose type is exactly what the cache erases and what the subcolumn's type is derived from. DataTypeTuple::equals() recurses through its elements into DataTypeNumber<UInt8>::equals(), a bare typeid test, so Tuple(x UInt8) and Tuple(x Bool) compare equal and MODIFY COLUMN between them is metadata-only with no mutation. The two parts then share one interned entry, whose subcolumn index was built from the winning parent, so p.x resolved through it reports the other part's type. A stale toString(p.x) granule holding '0'/'1' is accepted while the query now evaluates 'false'/'true'. Part loading is concurrent and shuffled, so which of the two types both parts report varies run to run. Resolve a subcolumn from the part's own parent instead: look up getNameInStorage() in the part's list and ask that type for the subcolumn via IDataType::tryGetSubcolumnType(). The pair passed in comes from IMergeTreeDataPart::tryGetColumn(), which has already split the name, because "a.b.c" is ambiguous between column a with subcolumn b.c and column a.b with subcolumn c and only the resolver knows which. When the parent is present but offers no such subcolumn, refuse rather than fall back to the cached type: that is a difference, not an unknown, and falling back would reintroduce the load-order dependency. The refusal is scoped to subcolumns so a top-level column keeps its existing fallback. Both routes into that refusal are currently closed upstream (ATTACH PARTITION FROM compares column lists with sizeOfDifference, and the ALTER validator rejects a tuple element rename that breaks a skip index), so it is fail-closed rather than test-covered.
…as staleness A skip index over a subcolumn is refused for a part whose own column list cannot describe that subcolumn. For a subcolumn of the DECLARED type that is right: the part recorded a different parent type, so the granule holds bytes of the old type. But a subcolumn can also exist only because the metadata-side type carries a custom SERIALIZATION. A Quantized codec column is the case in tree today: ColumnsDescription::attachQuantizeSerializationIfNeeded attaches SerializationQuantizedVector to that column's own type instance, and ColumnsDescription::addSubcolumns enumerates subcolumns through getDefaultSerialization(), which returns the custom serialization when one is set, so <col>.quantized is produced by the serialization and not by Array(Float32). A part's own NamesAndTypesList is reconstructed from columns.txt, which round-trips only the bare type name. Such a part therefore cannot describe <col>.quantized at all, and that silence is an unrepresentable-in-columns.txt fact rather than a type difference. Reading it as staleness made the index stop pruning for every part after a reload, so the same query answered differently before and after a restart. The refusal is reachable through two branches, and which one fires depends on part load order rather than on anything durable: no IDataType::equals() implementation compares custom_serialization, so a part whose own list is uncustomized shares the interned ColumnsDescription of a customized one. tryGetColumn() then succeeds while the part's own parent still offers no subcolumn. Both branches now ask the same question -- does the metadata-side parent carry a custom serialization -- so the answer cannot depend on which part loaded first. The predicate is the generic property, not a type name: nothing here knows about SerializationQuantizedVector, and no custom serialization is reconstructed on the query path. A subcolumn of the declared type is unaffected, so a Tuple element and a JSON typed path are still compared by the parent's equals() and a stale hint is still caught; a genuinely absent column, and a subcolumn whose parent the part does not carry at all, both still refuse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…parent The escape hatch that lets a part stay silent about a subcolumn its columns.txt structurally cannot express answered a weaker question than it needed to: "does the first resolvable dotted prefix of this name carry a custom serialization?". Two gaps followed. A required name was split even when it is itself a physical column, although ColumnsDescription::addSubcolumns documents that a physical b.x may coexist with a b that has subcolumns of its own. And a custom serialization on the resolved prefix was accepted without checking that the prefix actually defines the requested suffix - Bool carries one (SerializationBool) while defining no subcolumn at all, so an unrelated neighbour waved through a required column the part is simply missing, skipping every compatibility check for it and letting a stale granule prune all 16 granules and return 0 instead of 64. Both helpers now demand three things before the hatch fires: the exact name must not be a physical column, the resolved parent must carry a custom serialization, and that parent's own type must offer the requested suffix. A split that resolves but fails either type clause continues to the next split, so a dotted parent name such as `a.b`.quantized still resolves correctly. IDataType::tryGetSubcolumnType is the right primitive here because IDataType builds its SubstreamData from getDefaultSerialization(), which returns the custom serialization when one is attached. Also record in the getDeserializedFormat doc block that merge and mutate decide index carry-forward from file existence, not from that verdict. Found by Gate B review of the previous round. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The paragraph added last round lumped merge in with mutate. Mutate does decide carry-forward from file existence alone (MutateTask.cpp:441, :2235, :3180, and MergeTreeBlockReadUtils.cpp:67 for the read side), but merge's two text-index sites (MergeTask.cpp:2355 in MergeTextIndexStage::prepare, and :2932 in addBuildTextIndexesStep) both call getDeserializedFormat and are governed by this verdict, so a part whose text index it refuses is rebuilt during the merge instead of being carried forward. Comment only, no behaviour change.
The escape hatch for a subcolumn that only a parent's custom serialization defines asked whether the metadata-side parent defines the suffix, but not whether THIS part holds that parent at all. A part that never received the parent therefore skipped the type check entirely, and its granule, written for the column's older type, was used to prune. Reachable today through a backticked index expression. `ADD INDEX idx `vec.8`` is one identifier rather than the dot operator, so the index requires the QBit bit-plane SUBCOLUMN, which no part list can express; on a part that carries no `vec`, a killed `MODIFY COLUMN vec QBit(Float64, 4)` then left the stale granule in use and the indexed count answered 38 against an unindexed 62. The unbackticked `vec.8` spelling requires the physical parent instead and was already correct, which is why the earlier carrier sweep over types missed this: reachability follows the spelling, not the type. `hasSerializationDefinedSubcolumns` now also requires the part's own column list to carry the resolved parent, and both call sites ask the identical question, since which of them fires depends on part load order. The predicate names no type: the discriminator is parent presence. A subcolumn the part cannot express for a parent it does hold still keeps pruning.
The previous commit message wrote the backticked index name inside a single-backtick span, which
cannot contain a backtick. The expression is:
ALTER TABLE t ADD INDEX idx `vec.8` TYPE set(100) GRANULARITY 1
No change to the tree.
Case 24 asserts that a skip index over a serialization-defined subcolumn whose parent the part does not carry refuses to prune. On its own that assertion is also satisfied in a world where a backticked QBit subcolumn index never prunes at all, so it cannot attribute the refusal to parent absence. The in-tree coverage does not close the gap either: 04403 declares its index with the unbackticked vec.8, which is the dot operator and requires the physical parent, so it never exercises this spelling. A future change refusing every backticked QBit subcolumn index would leave case 24 green while shipping a pruning regression. Case 25 adds the missing side: the same backticked index on a part that does carry the parent, with no stale type, must still prune. The probe byte and the granule count were measured on the fixture rather than copied - of the nine distinct vec.8 values only 02 is selective, giving Granules: 1/16, while case 24's 00 spans ten granules and would have made the assertion nearly unfalsifiable. Each half of the pair is now separately load-bearing. Dropping the parent-presence clause, so the escape hatch is taken unconditionally, reddens only case 24; refusing a serialization-defined subcolumn on the type-compatible path reddens only case 25. Two comments are corrected. Case 24's claimed that a QBit bit plane cannot be expressed in any part list, which is false: DataTypeQBit's constructor sets its custom serialization unconditionally and NamesAndTypesList::readText rebuilds the type through DataTypeFactory::get, so a reconstructed parent does offer the subcolumn. The reason to refuse is parent absence; the unrepresentable case is Quantized, whose serialization comes from a codec that columns.txt drops. Case 21 records that MergeTreeData::checkAlterIsPossible rejects any ALTER of a Quantized codec, so no stale-type shape is constructible for that carrier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
Internal second-model review12 review rounds, 68 findings adjudicated. Final round: 1 blocker refuted by measurement, 2 self-fixes applied. Click to expand.This PR was written by one model and reviewed by a different one on every round, with the reviewer ❌ Refuted with evidence
The third arm is decisive: Root cause is separate: A prescribed mutation was structurally incapable of testing what it was asked to test. The fix
|
Collaborator
Author
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-44:20260729-043002 |
This was referenced Jul 29, 2026
Collaborator
Author
|
cc @shankar-iyer @ahmadov, could you review this? The gate that should exclude a skip index whose column type changed infers staleness from a pending mutation entry, so a killed mutation (or a conversion that creates no mutation) leaves index analysis decoding a part's old bytes with the new type; this compares the part's own recorded type against the metadata type inside getDeserializedFormat instead, which also covers the text index and both merge-path sites. |
alexey-milovidov
added
the
can be tested
Contributor
|
Workflow [PR], commit [3623a24] Summary: ✅
AI ReviewSummaryThis PR moves skip-index usability checks into Findings❌ Blockers
Tests
Final VerdictChanges requested: the stale-type gate still has a same-name shadowing case that can misprune rows after a killed LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 94/106 (88.68%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Collaborator
Author
CI finish ledger - b3e80b1Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
The Session id: cron:our-pr-ci-monitor:20260730-163853 |
Resolves the semantic overlap with the now-merged ClickHouse#109616, which introduced IMergeTreeIndex::getAllSubstreamsInPart. That accessor answers the physical question independently (its own getSubstreams + indexFileExistsInChecksums walk, with an explicit comment that it is deliberately not routed through getDeserializedFormat), so it needs no redirection onto this branch's new getPhysicalFormat: the union-of-on-disk-versions property that mutation cleanup and size accounting depend on holds by construction after the merge. The only textual conflict was the MergeTreeIndexMinMax declaration block, where this branch renames the virtual to getPhysicalFormat and master adds a getAllSubstreamsInPart override. Both are kept. Master adds no new getDeserializedFormat call site (13 before, 13 after), so the usability-versus-physical audit of this branch still covers every caller.
Merging master shifted both getDeserializedFormat calls in MergeTask.cpp, so the line numbers this comment quoted no longer resolved. Naming the enclosing operations keeps the reference valid across unrelated churn in that file.
…heck The compatibility helper unwrapped Array and Nullable but not LowCardinality, so a dictionary-side representation-preserving conversion such as LowCardinality(DateTime) to LowCardinality(UInt32) was judged stale and pruning was skipped on every affected part although the granules stay valid. Measured on the merge base the index prunes 1/16 granules there; without this it reads 16/16. LowCardinality keeps its own framing (a dictionary plus indexes) whatever the dictionary type is, and SerializationLowCardinality forwards value bytes to the dictionary type's own serialization, so the dictionary-side question is exactly the one the existing allow list already answers. The unwrap is pairwise like the wrappers above it. Adding or dropping the wrapper changes the framing itself rather than the values, and no allow-list entry may wave that through: with a one-sided unwrap the dropped-wrapper case reads a dictionary as if it were values and fails with PARAMETER_OUT_OF_BOUND, which is what master does today. Cases 26 and 27 cover the restored pruning, 28 and 29 pin the refusal. Reported by clickhouse-gh[bot] on ClickHouse#112484.
The mechanism (LowCardinality forwards value bytes to the dictionary type's own serialization, so a dictionary-side representation-preserving conversion leaves every byte alone) belongs in the commit message, not beside the code. What the reader needs at this line is why the match has to be pairwise.
Cases 28 and 29 asserted only the result, which also passes when a stale index is used but happens not to misprune the probe value. Both now assert that no granule is dropped, so the index is provably refused rather than merely harmless here. That also makes case 28 catch the one-sided unwrap: dropping the pairwise requirement reintroduces the PARAMETER_OUT_OF_BOUND master produces today, and the new granule assertion is where the test now fails. The two cases refuse with or without the LowCardinality branch, so their comment no longer claims otherwise.
Contributor
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 99/107 (92.52%) · Uncovered code |
This was referenced Aug 14, 2026
Member
|
Can't wait... I will also take it for review. |
alexey-milovidov
approved these changes
Aug 15, 2026
robot-ch-test-poll
added
the
pr-synced-to-cloud
This was referenced Aug 15, 2026
Contributor
|
@groeneai @alexey-milovidov @shankar-iyer — ClickGap found the following in this PR:
Close anything that's wrong or already addressed. |
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
Aug 17, 2026
ClickHouse#112484 renamed the virtual IMergeTreeIndex::getDeserializedFormat to getPhysicalFormat and made getDeserializedFormat a non-virtual wrapper that adds usability checks. That rewrote the same four sites this branch touches. Resolution: keep master's rename everywhere (both overrides and the doc block), keep this branch's getPotentialSubstreams beside it. The eviction call site drops master's getPhysicalFormat call entirely, which is the point of the branch: that call is what does object-storage I/O during part destruction. getPhysicalFormat keeps its other caller in getDeserializedFormat, so nothing is orphaned. getPotentialSubstreams remains a superset of every layout getPhysicalFormat can report, for both overriding index types: minmax returns .idx2 and legacy .idx against master's two probes, and text returns .idx/.dct/.pst/.pos against master's v1/v2 pair. The evicted keys therefore still cover every key loadIndexMarksToCache can insert, which is the contract that could break silently. Also folded master's NOTE on getSubstreams into one sentence naming both methods, and updated the gtest comment to the new method name.
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
Aug 17, 2026
Two things the master merge left inconsistent, both comment-only. The doc block introduced by ClickHouse#112484 names "evicting index marks" as the exemplar caller that needs physical discovery, but this branch removes exactly that caller: resolving what a part holds probes the part's storage, which must not happen while destroying a part. Left as-is the comment invites reintroducing the object-storage read it exists to prevent, so it now names the constraint and points at getPotentialSubstreams instead. CLAUDE.md also asks for `f` rather than `f()` when a comment refers to a function itself; applied to the lines this branch writes. Pre-existing text elsewhere in these files is left alone so the diff stays about the substream API.
This was referenced Aug 20, 2026
Open
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
Sep 1, 2026
Refresh the branch against master d466df7 so it is ready to review. The two CI failures the finish ledger owned have since been fixed on master (ClickHouse#108855 and ClickHouse#112484), which is what was blocking Mergeable Check. No conflict: the clone() triple copy still applies verbatim and master's newer restores in the same function are preserved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixes reading a stale secondary (skip) index after an
ALTER TABLE ... MODIFY COLUMNtype change whose mutation never ran, for example becauseKILL MUTATIONremoved it: the granules on disk were written with the old type but decoded with the new one, raisingLOGICAL_ERROR, requesting multi-exabyte allocations, or silently returning a wrong result. Also fixes a wrong result from an index over an expression whose meaning changes while every stored byte stays identical, so no mutation is created at all: aMODIFY COLUMNaltering only aDateTimetimezone, or only a custom type name such asUInt8toBool. Such an index is now skipped for the affected part. Closes #112213.Description
MODIFY COLUMNupdates table metadata at once and schedules a mutation to rewrite the parts. Until it runs, a part's granules hold bytes written with the OLD type while index analysis decodes them with the NEW one.The read gate
canUseIndexinfers that mismatch from a pending mutation entry and fails open when there is none. Six measured ways reach the deserializer anyway, includingKILL MUTATIONhaving removed the entry (the report) and conversions for which no mutation is ever created, being metadata-only for the column but not for the index. The top-k minmax read had no gate at all.The fix asks the durable question instead: do the part's bytes match the type about to decode them? It lands in
IMergeTreeIndex::getDeserializedFormat, which already receives the part, so one predicate covers every read path. Physical discovery splits off into a virtualgetPhysicalFormat, leavinggetDeserializedFormatnon-virtual so no override bypasses it. Each required column's part-side type, taken from the part's own list rather than the type-erasing interned cache, is compared against the metadata type: only representation-preserving differences pass, plus agetName()same-meaning check on the equals-equal path. The walk recurses pairwise throughArray,NullableandLowCardinality; adding or dropping a wrapper is refused. A non-trivial expression index is refused on any difference.Over-firing costs pruning, not correctness. Two
MergeTasktext-index sites share the predicate, so a stale text index is rebuilt during a merge instead of hardlinked forward. Out of scope: index identity staleness (a changed expression, a name reused after a killedDROP INDEX), which no type check detects.Measured symptoms on master, and validation
Nullable(UInt64)LOGICAL_ERROR: Sizes of nested column and null map ... not equal after deserializationNullable(UInt64)release /UInt64/ absent-column partCode: 241, 4 / 2 / 4 EiBArray(UInt64)Code: 33, "read just 38 of 18005230136"Int8toEnum8Code: 691)DateTime('UTC')toDateTime('Asia/Tokyo'),INDEX toHour(dt)UInt8toBool,INDEX toString(v)Tuple(x UInt8)toTuple(x Bool),INDEX toString(p.x)04165_skip_index_stale_type_after_alter.sql: 29 cases covering the above, whose 21 controlfixtures carry 25
explain ILIKEgranule assertions pinning the over-fire direction (a simplesingle-column index across the timezone and
BoolALTERs, an unchanged subcolumn index, anunchanged
JSON(a DateTime)column).On the base commit the test kills the server with the reported stack
(
SerializationNullable.cpp:178<-MergeTreeIndexGranuleSet::deserializeBinary<-MergeTreeIndexReader::read<-filterMarksUsingIndex); with the fix it passes 50/50.36 mutations each confirm one line is load-bearing. A 462-test A/B sweep against pure HEAD leaves
the failure set unchanged (27 in both arms, all needing infra this sandbox lacks). Perf over 250
parts and 4 indexes is inside noise. No setting or format changes. #110050 overlaps these files and its helper asks the
physical question, so it should forward to
getPhysicalFormaton rebase. My #109616 has sincemerged and independently reached the same physical-vs-usability split, so it needs no forwarding.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108096&sha=25f51f1fddd05350d9c5aa1231aa0e7ee6fc676f&name_0=PR&name_1=Stress%20test%20%28arm_tsan%29
Version info
26.8.1.1470(included in26.8and later)