Promote ConstantValue to Core with Field-free value accessors - #114666
Open
yakov-olkhovskiy wants to merge 3 commits into
Open
Promote ConstantValue to Core with Field-free value accessors#114666yakov-olkhovskiy wants to merge 3 commits into
yakov-olkhovskiy wants to merge 3 commits into
Conversation
Continue removing `DB::Field` from constant handling by making the owning
single-constant type (`{size-1 ColumnConst, DataTypePtr}`) a shared primitive
instead of an Analyzer-local helper passed around as a raw pair.
- Move `ConstantValue` from `Analyzer/` to `Core/`. It carries a `DataTypePtr`,
and `DataTypes` depends on `Columns`, so `Core` is the correct home (it also
fixes a lower layer having to reach up into `Analyzer` for the type).
- Add value accessors that read row 0 of the size-1 column without materializing
a `Field`: `isNull`, `getUInt`, `getInt`, `getFloat64`, `getBool`, `getDataAt`.
`ConstantNode` gains matching delegators, and its `getValue` now delegates to a
single transitional `ConstantValue::getField`.
- `evaluateConstantExpressionAsColumn` now returns a `ConstantValue` instead of a
`std::pair<ColumnPtr, DataTypePtr>`; all callers are updated (table functions,
`ActionsVisitor`, `InterpreterSelectQuery`, prometheus/timeSeries selectors,
gtest). The two `getStringConstArgument` helpers read the value directly via
`isNull`/`getDataAt`.
No user-visible behavior change: the value is the same size-1 const column with
the same exact type, just bundled. `ConstantValue`'s `Field` constructor and
`getField` remain as the sole `Field` entry/exit while the analyzer still folds
constants into `Field`s (a later phase removes them).
Verified: `gtest_convert_column_to_type`, `gtest_evaluate_constant_expression`,
and a `numbers`/`generate_series`/`values`/`LIMIT`/`IN` stateless spot-check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Workflow [PR], commit [000cc7f] AI ReviewSummaryThis PR promotes Missing context / blind spots
Final VerdictNo new findings. |
clickhouse-gh
Bot
added
the
pr-not-for-changelog
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Contributor
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units183 translation units recompiled, 1601 s compile time in total, 183 of them have a recent master baseline. |
Address review: `evaluateConstantExpression.h` is included by ~100 translation units (several via other public headers). Including `Core/ConstantValue.h` there pulled `Columns/ColumnConst.h` + `DataTypes/IDataType.h` (and their transitive closure) into every includer - a repo-wide compile-time regression for what is a local change. The header only *names* `ConstantValue` (the `EvaluateConstantExpressionColumnResult` alias and the two function return declarations, including the `std::optional<...>` one), none of which need a complete type. Forward-declare `class ConstantValue;` in the header and keep the full include in `evaluateConstantExpression.cpp` and in the caller TUs that receive the value by value (table functions, `ActionsVisitor`, `InterpreterSelectQuery`, prometheus/timeSeries selectors, gtest). No behavior change. Verified by a full rebuild (all includers compile with the incomplete type) plus the constant-expression / convert gtests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review: `materializeToField` is the compatibility bridge that preserves the pre-refactor `Field` semantics for NON-literal constants, and it historically read the value via `(*result_column)[0]` (the column's `operator[]`). The ConstantValue refactor switched it to `ConstantValue::getField()`, which uses `IColumn::get`. For `ColumnConst`, `operator[]` and `get` dispatch to the data column's separate `operator[]`/`get` virtuals - not guaranteed identical for wrapped columns - so a bridge meant to stay byte-for-byte compatible must use the same call it always did. Read the value here via `(*column_result->getColumn())[0]` again. `ConstantValue::getField()` (which uses `get`) stays for `ConstantNode::getValue`, matching that caller's own historical `get` path. Add `EvaluateConstantExpression.NonLiteralFieldBridge`: on the analyzer path a non-literal constant reaches this bridge; it checks value preservation, the `Bool`->`UInt64` canonicalization, and a `Nullable` unwrap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Continue removing
DB::Fieldfrom constant handling by promoting the owning single-constant type to a shared primitive withField-free value accessors.What
ConstantValuefromAnalyzer/toCore/. It bundles{size-1 ColumnConst, DataTypePtr}. It carries aDataTypePtr, andDataTypesdepends onColumns, soCoreis the correct layer (this also removes a lower layer having to reach up intoAnalyzerfor the type). It stays deliberately distinct fromColumnWithTypeAndName(size-1 const invariant, no name, scalar accessors).Field:isNull,getUInt,getInt,getFloat64,getBool,getDataAt.ConstantNodegains matching delegators, andConstantNode::getValuenow delegates to a single transitionalConstantValue::getField.evaluateConstantExpressionAsColumnnow returnsConstantValueinstead ofstd::pair<ColumnPtr, DataTypePtr>. Callers updated:numbers/primes/generateSeries/valuestable functions,ActionsVisitor,InterpreterSelectQuery(LIMIT/OFFSET), prometheus/timeSeries selectors, and the gtest. The twogetStringConstArgumenthelpers now read the value directly viaisNull/getDataAt.Behavior
No user-visible change: the value is the same size-1 const column with the same exact type, just bundled and readable without a
Field.ConstantValue'sFieldconstructor andgetFieldremain as the onlyFieldentry/exit while the analyzer still folds constants intoFields; a later phase removes them.Verification
gtest_convert_column_to_type,gtest_evaluate_constant_expression, and anumbers/generate_series/values/LIMIT(integer + fractional)/INstateless spot-check.Related: #113051
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Not for changelog: internal refactor toward removing
DB::Field; no user-visible behavior change.