Type the _partition_value virtual column so it can hold the values it exposes - #119394
Type the _partition_value virtual column so it can hold the values it exposes#119394groeneai wants to merge 5 commits into
Conversation
… exposes The values a part stores in MergeTreePartition::value are produced and persisted by the ADJUSTED partition key, in which `modulo` is rewritten to `moduloLegacy` for on-disk compatibility (MergeTreePartition::adjustPartitionKey). _partition_value's declared type was derived from the DECLARED key instead. The two keys disagree because the result traits differ: ResultOfModulo takes signedness from the left operand alone and widens only when that operand is signed, while ResultOfModuloLegacy takes signedness from either operand and never widens. So exactly one divergence exists, an unsigned left operand with a signed right one, and it has two faces. Below 128 bits Field::safeGet treats Int64 and UInt64 as interchangeable, so ColumnVector<UInt32>::insert accepts a Field holding an Int32 and reinterprets the bits: a negative produced value surfaces as a large positive one. At 128/256 bits there is no such leniency and the discriminant mismatch throws BAD_GET. An element now keeps its declared type when that type can represent every produced value (canBeSafelyCast), and takes the produced type otherwise. A signedness divergence is never representable; a pure widening is, so an ordinary `PARTITION BY <signed> % <small>` key keeps the type it announces today. Typing the whole tuple by the produced key was rejected: it would move 01848_partition_value_column's declared element from Int16 to Int8 for no defect. Two carriers declare the column and both are fixed: the MergeTreeData constructor and fillProjectionDescriptionByQuery. The three sites that materialize it read the declared type, so they need no change. MergeTreePartition::getID is deliberately untouched, because it would change partition IDs. adjustPartitionKey gained an overload taking the key, columns and virtuals directly, since the MergeTreeData constructor has no metadata snapshot yet, its virtuals being what is under construction. Passing createVirtuals(nullptr) as the virtuals for that analysis mirrors registerStorageMergeTree, which analyses the declared PARTITION BY against exactly that set, and cannot lose anything: a key naming this column is rejected today with UNKNOWN_IDENTIFIER, so no existing key can reference the column being declared. Third and last layer of this mismatch. The parse side was fixed in ClickHouse#116606 and the render side is ClickHouse#119333. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
04952 declared a single-element partition key in every scenario, so the per-element rule was indistinguishable from an all-or-nothing one: keeping the whole declared tuple only when every element is representable, and otherwise replacing the whole tuple, passes all eight of its assertions. A mixed key now pins the difference. Its first element diverges (declared UInt128, produced Int128) and takes the produced type, while its second merely widens (declared Int16, produced Int8) and keeps its declared one, so the expected type is Tuple(Int128, Int16); an all-or-nothing rule gives Tuple(Int128, Int8) and changes only this line. PartitionValueColumn::type now bounds its loop by the smaller of the two sizes. The chassert above it is debug-only, so indexing the produced types by the declared count was unguarded in a release build. Not reachable today, because moduloToModuloLegacyRecursive renames ASTFunction::name and preserves arity, so this only stops a future rewrite that does not. Nothing here widens what the produced-key analysis accepts, and it does not need to: moduloLegacy accepts strictly fewer operand pairs than modulo, since IsOperation<ModuloLegacyImpl>::modulo is false and both division and allow_decimal key off it, but registerStorageMergeTree already runs that same legacy analysis on every table registration when it builds the implicit minmax-count projection. A key whose legacy form cannot be analysed, such as a Date left operand, therefore already fails to load one layer earlier with the same ILLEGAL_TYPE_OF_ARGUMENT; deriving the produced key in the MergeTreeData constructor adds no way for a table to stop loading. Measured across 22 operand shapes, including Date, Date32, DateTime, DateTime64 and Decimal left operands and an explicit projection over such a key: accepted and rejected sets are identical with and without this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Tuple and Array The whole minimality guarantee of this change rests on canBeSafelyCast, which recurses through Nullable and LowCardinality on both source and target and into Tuple and Array element types. Every partition key 04952 declared was a plain numeric type, so a wrapper-specific degradation inside that recursion would have mistyped every wrapped key while reddening nothing. Five keys now pin it. A Nullable divergent key is re-typed inside the wrapper (Tuple(Nullable(UInt32)) before, Tuple(Nullable(Int32)) after) and a Nullable widening key keeps its declared type inside it, which is the pair that shows the choice is made under the wrapper rather than on it; LowCardinality behaves the same way. The Array key covers the element-container arm. Reaching the Tuple recursion needs a doubled tuple(): a top-level tuple() IS the key list, so PARTITION BY tuple(a % b, c) is two top-level elements and exercises only the per-element loop, while PARTITION BY tuple(tuple(a % b, c)) leaves the inner tuple as one element's own type. That key declares Tuple(Tuple(UInt32, Int32)) before this change and Tuple(Tuple(Int32, Int32)) after. Four of the five are therefore red on an unfixed binary. The fifth, the Nullable widening key, is deliberately unchanged by this fix and guards the opposite direction: replacing every element with its produced type moves it to Tuple(Nullable(Int8)), and moves only the two other widening lines with it. mod_negative also asserts its type now. It is the file's most extreme divergence, a declared UInt128 against a produced Int128 holding -1, and the value alone would survive a rule that picked any other signed type at least 128 bits wide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
groeneai
added
can be tested
Internal second-model reviewTwo independent passes over the final tree (a cold read, then a second model). Two items are worth
💡 Nested tuples are replaced whole, not per member. The choice is made once per top-level key |
|
Workflow [PR], commit [047bf17] Summary: ❌
AI ReviewSummaryThis PR retunes Final Verdict✅ No blocking or major issues found. LLVM Coverage ReportMeasured on commit 047bf17.
Changed lines: Changed C/C++ lines covered: 39/39 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
04952 creates tables whose partition key diverges at 128 bits, which is the defect it asserts. Inserting a part into one of them also makes its part_log entry render the partition with the declared key (PartLog::addNewPartsImpl -> MergeTreePartition::serializeToString(metadata)), which throws BAD_GET and logs it at ERROR. clickhouse-test runs the client with --send_logs_level=warning and fails any test whose stderr is not empty, so Fast test reported "having stderror" for all four of those inserts while every assertion matched. Rendering with the produced key is the render layer's fix, ClickHouse#119333, whose own test covers the part_log carrier, so this does not widen the diff into it. send_logs_level = 'fatal' is what the suite already uses for expected server-side log noise; it keeps <Fatal> visible, and CI's server-log scan looks for <Fatal>, not <Error>. Measured: at the runner's own level the test's stderr is empty; with the client raised to trace, forwarded traffic drops from 90209 bytes to the SET statement's own <Debug> line, which is itself below the runner's level. Assertions and the reference file are unchanged, 50/50 randomized. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
clickhouse-gh
Bot
added
the
comp-mergetree
PartitionValueColumn::type makes one choice per top-level key element, and canBeSafelyCast rejects a whole tuple as soon as one of its members is not representable, so a divergent member takes the entire element to the produced type rather than only itself. The comment claimed the sibling member was left alone, which is not what happens: with the sibling changed to a merely widening expression, the key reports Tuple(Tuple(Int32, Int8)) rather than Tuple(Tuple(Int32, Int16)), while the same two expressions as separate top-level elements report Tuple(Int32, Int16). Comment only. mod_nested's sibling is Int32 in both keys, so the shipped assertion is unchanged; the per-element choice at the top level is already pinned by mod_mixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units297 translation units recompiled, 2755 s compile time in total, 297 of them have a recent master baseline. |
CI finish ledger - 047bf17Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task whose
Session id: cron:our-pr-ci-monitor:20260911-120034 |
1 participant
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed the
_partition_valuevirtual column for aPARTITION BYwithmodulowith an unsigned left and a signed right operand. Such a key stores the valuesmoduloLegacycomputes, whose signedness differs from the declared type, so a negative stored value was reported as a large positive one below 128 bits, and reading it threwBad get: has Int128, requested UInt128 (BAD_GET)at 128/256-bit widths. The declared element type is now one that can hold the stored values, so for such a key it changes (e.g.Tuple(UInt32)toTuple(Int32)) even where the value was already correct.Description
_partition_valuewas typed from the declaredPARTITION BY, but a part's values are produced and persisted by the adjusted key, wheremodulobecomesmoduloLegacyfor on-disk compatibility.ResultOfModulotakes signedness from its left operand alone,ResultOfModuloLegacyfrom either, so an unsigned left with a signed right operand makes declaration and data disagree.A positive literal is unsigned, so this needs no
CAST:The part is
-1294967296: the column contradicted its own part's name. Below 128 bitsField::safeGettreatsInt64/UInt64as interchangeable and the bits were reinterpreted; at 128/256 bits it threwBAD_GET. A predicate on the column reaches the same path, so this is not confined to queries that select it.Fix. An element keeps its declared type when that type can represent every produced value (
canBeSafelyCast), else takes the produced type. A signedness divergence never can; a widening can, so an ordinaryPARTITION BY <signed> % <small>key is untouched. Both carriers that declare the column (the table's virtuals and projection metadata) are fixed; the three sites materializing it read the declared type.MergeTreePartition::getIDis untouched, so partition IDs do not change, and no on-disk format or setting changes.Validation. New test
04952, both directions per carrier with distinct build IDs, the wrong value checked againstsystem.parts.partition_idas an independent oracle;Nullable,LowCardinality, nested-tuple and array wrapper keys; 50/50 randomized. Thepartitionandprojectionsubsets moved no reference, and each failure there reproduces on a base binary or carries a documented sandbox signature.Third layer of this mismatch: #116606 fixed the parse side, #119333 the render.
Workflow [PR]
Sync PR [sync-upstream/pr/119394]