Fix accurate casts from an unsigned integer to IPv4 and IPv6 - #113040
Fix accurate casts from an unsigned integer to IPv4 and IPv6#113040groeneai wants to merge 2 commits into
Conversation
accurateCast, accurateCastOrNull and accurateCastOrDefault never converted an unsigned integer to IPv4, nor UInt128 to IPv6, even though the plain CAST of the same value succeeds. accurateCastOrNull returned NULL, accurateCast threw CANNOT_CONVERT_TYPE, and accurateCastOrDefault returned 0.0.0.0. Because the MergeTree set index builds its pruning set with castColumnAccurateOrNull, this silently returned wrong results: an IPv4 primary key compared with IN against a UInt32 subquery pruned every part, since every set element became NULL. Set also uses the accurate casts, so a runtime IN over the same cross-type pair returned 0 while = returned 1, and under transform_null_in = 1 the same query threw instead. FunctionCast::createWrapper enters its accurate branch for any integer source, then dispatches into a lambda whose value-producing arms are guarded by IsDataTypeNumber<RightDataType>. That trait is specialized only for DataTypeNumber<T>, and DataTypeIPv4/DataTypeIPv6 are not, because their field type is a StrongTypedef. No arm matched, so the wrapper fell through to an all-NULL column or a throw without ever examining the value. The plain CAST was unaffected because it reaches ConvertImpl, which already whitelists UInt8/UInt16/UInt32/UInt64 as IPv4 sources. This adds the missing dispatcher arm plus an accurate integer-to-IPv4 conversion. The conversion range-checks against IPv4::UnderlyingType rather than the StrongTypedef itself, mirroring convertFieldToType, which converts a UInt64 field to IPv4 through UInt32; accurate::convertNumeric cannot be instantiated for a StrongTypedef target at all. The check is load-bearing: the existing convertFromUInt64ToIPv4 truncates, so reusing it would have replaced NULL with a silently wrong value. UInt128 to IPv6 needs only the dispatcher arm, since that conversion already handles accurate additions; it is left untouched because IPv6 is stored big-endian and its helper byte-swaps both limbs. Fixing the conversion layer corrects every consumer at once: Set, KeyCondition, castOrDefault, KVStorageUtils, FunctionsJSON and evaluateConstantExpression. Plain CAST keeps its behaviour, including truncating values above 2^32, because the new ConvertImpl arm is selected only for the accurate additions types. Scope was established by sweeping all 702 ordered pairs over 27 types against the invariant "plain CAST succeeds, so the accurate cast must not return NULL". Exactly 6 pairs violated it (UInt8/UInt16/UInt32/UInt64/Bool to IPv4, UInt128 to IPv6) and all 6 are fixed with no residue. UUID and the Int*, Float* and UInt256 sources are deliberately out of scope: plain CAST throws for those too, so returning NULL is consistent, and making them convertible would be a new capability rather than a bug fix. The new test pins them as negative controls. 03212_variant_dynamic_cast_or_default is updated because toIPv4OrDefault and toIPv6OrDefault over a Dynamic column build on the accurate cast, so values that previously fell to the type default now convert, matching what plain CAST returns for the same inputs. Its diagnostic allow-list is widened for the same reason rather than removed. This is the same update c9e6de1 made to that reference when the accurate cast to Date32 became strict. Related: ClickHouse#111418
Internal second-model review (0 blockers, 0 majors)Reviewed independently of the author: a cold read of the resulting code first (carriers enumerated Result: 0 blockers, 0 majors. The second-model pass returned no findings. Five nits from the
Independently verified during the cold read, rather than taken from the author's notes:
Session id: cron:clickhouse-review-slot-48:20260802-220600 |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-4:20260802-182400 |
|
cc @Ergus @KochetovNicolai, could you review this? |
|
Workflow [PR], commit [3c46800] Summary: ❌
AI ReviewSummaryThis PR fixes the accurate-cast dispatcher so unsigned-integer to Final Verdict✅ No blockers or majors found in the current version. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 43/46 (93.48%) · Uncovered code |
Build (arm_tidy) failed with cppcoreguidelines-init-variables, which is enabled with -warnings-as-errors: the loop-local out-parameter passed to accurate::convertNumeric was declared without an initializer. The header is included by 35 translation units, so the single declaration produced 38 diagnostics and dropped the other 151 jobs of the workflow. No behavior change: convertNumeric assigns the variable on every success path, and the failure path never reads it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Build profile diff (arm_release)Comparing 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. Object file sizes
|
| Object file | Master | PR | Δ |
|---|---|---|---|
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/ext/transport/chttp2/transport/cht… |
960.56 KiB | removed | -960.56 KiB (-100.00%) |
src/Common/ZooKeeper/CMakeFiles/clickhouse_common_zookeeper_no_log.dir/ZooKeeperImpl.cpp.o |
804.97 KiB | removed | -804.97 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/server/server.cc.o |
612.22 KiB | removed | -612.22 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/load_balancing/rls/rls.cc.o |
611.36 KiB | removed | -611.36 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/client_channel/client_channel_filt… |
548.04 KiB | removed | -548.04 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/client_channel/client_channel.cc.o |
525.46 KiB | removed | -525.46 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/client_channel/retry_interceptor.c… |
518.85 KiB | removed | -518.85 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/lib/channel/promise_based_filter.c… |
482.91 KiB | removed | -482.91 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/load_balancing/grpclb/grpclb.cc.o |
434.97 KiB | removed | -434.97 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/call/client_call.cc.o |
433.89 KiB | removed | -433.89 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/call/server_call.cc.o |
391.29 KiB | removed | -391.29 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc++_unsecure.dir/__/grpc/src/cpp/server/server_cc.cc.o |
384.88 KiB | removed | -384.88 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/client_channel/retry_filter_legacy… |
369.55 KiB | removed | -369.55 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/ext/transport/chttp2/transport/wri… |
349.81 KiB | removed | -349.81 KiB (-100.00%) |
contrib/google-cloud-cpp-cmake/CMakeFiles/google_cloud_cpp_grpc_utils.dir/__/google-cloud-cpp/googl… |
346.93 KiB | removed | -346.93 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/ext/transport/inproc/legacy_inproc… |
345.35 KiB | removed | -345.35 KiB (-100.00%) |
contrib/google-cloud-cpp-cmake/CMakeFiles/google_cloud_cpp_longrunning_operations_protos.dir/google… |
339.33 KiB | removed | -339.33 KiB (-100.00%) |
contrib/google-cloud-cpp-cmake/CMakeFiles/google_cloud_cpp_iam_credentials_v1_iamcredentials_protos… |
334.24 KiB | removed | -334.24 KiB (-100.00%) |
contrib/google-protobuf-cmake/CMakeFiles/_libprotobuf-lite.dir/__/google-protobuf/src/google/protob… |
316.58 KiB | removed | -316.58 KiB (-100.00%) |
contrib/grpc-cmake/CMakeFiles/grpc_unsecure.dir/__/grpc/src/core/call/call_spine.cc.o |
312.88 KiB | removed | -312.88 KiB (-100.00%) |
Slowest function optimization changes (ThinLTO) ⚠️
Median per-function time ratio to the master baseline is ×1.11 (different machine and build flags); deltas below are relative to that ratio.
| Binary | Function | Master | PR | Δ vs median |
|---|---|---|---|---|
programs/clickhouse-keeper |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.11069338206770680048] |
new | 18.4 s | +18.4 s |
programs/clickhouse |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.12487241783113815285] |
9.0 s | gone | -10.0 s (-100%) |
programs/clickhouse-keeper |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.12487241783113815285] |
8.5 s | gone | -9.4 s (-100%) |
programs/clickhouse |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.11069338206770680048] |
new | 7.6 s | +7.6 s |
programs/clickhouse-keeper |
DB::registerFunctionConversion(DB::FunctionFactory&) |
7.1 s | 15.3 s | +7.4 s (+95%) |
programs/clickhouse-keeper |
_GLOBAL__sub_I_TargetLibraryInfo.cpp |
5.8 s | 12.1 s | +5.6 s (+87%) |
programs/clickhouse |
DB::Parquet::writeColumnChunkBody(DB::Parquet::ColumnChunkWriteState&, DB::Parquet::WriteOptions co… |
5.5 s | 3.4 s | -2.7 s (-45%) |
programs/clickhouse-keeper |
DB::ServerSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.54623650351436… |
new | 2.7 s | +2.7 s |
programs/clickhouse-keeper |
DB::MergeTreeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.56540966842… |
new | 2.5 s | +2.5 s |
programs/clickhouse-keeper |
_ZN2DB18FunctionArrayIndexINS_16CountEqualActionENS_14NameCountEqualEE15executeIntegralIJDutjmDB8_s… |
1.9 s | 4.5 s | +2.4 s (+112%) |
programs/clickhouse |
DB::getSettingsChangesHistory()::$_0::operator()() const |
4.6 s | 2.7 s | -2.4 s (-46%) |
programs/clickhouse-keeper |
DB::registerFunctionVectorFunctions(DB::FunctionFactory&) |
2.3 s | 4.9 s | +2.3 s (+93%) |
programs/clickhouse-keeper |
_GLOBAL__sub_I_sqid.cpp |
1.7 s | 4.3 s | +2.3 s (+121%) |
programs/clickhouse |
DB::ServerSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.17566710545920… |
1.9 s | gone | -2.1 s (-100%) |
programs/clickhouse |
DB::Aggregator::writeToTemporaryFile(DB::AggregatedDataVariants&, unsigned long) const |
3.6 s | 1.9 s | -2.1 s (-53%) |
Compile time of recompiled translation units
40 translation units recompiled, 268 s compile time in total, 40 of them have a recent master baseline.
Median compile-time ratio to the baselines is ×1.07 (machine-speed difference or a change affecting every TU); per-TU deltas below are relative to that ratio.
The matched translation units cost +22.0 s (+9%) in total before that adjustment.
Symbol sizes ⚠️
| Binary | Symbol | Master | PR | Δ |
|---|---|---|---|---|
programs/clickhouse |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.11069338206770680048] |
new | 439.45 KiB | +439.45 KiB |
programs/clickhouse-keeper |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.11069338206770680048] |
new | 439.45 KiB | +439.45 KiB |
programs/clickhouse |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.12487241783113815285] |
439.45 KiB | removed | -439.45 KiB (-100.00%) |
programs/clickhouse-keeper |
DB::SettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.12487241783113815285] |
439.45 KiB | removed | -439.45 KiB (-100.00%) |
programs/clickhouse |
DB::ServerSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.17566710545920… |
114.16 KiB | removed | -114.16 KiB (-100.00%) |
programs/clickhouse |
DB::ServerSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.54623650351436… |
new | 114.16 KiB | +114.16 KiB |
programs/clickhouse-keeper |
DB::ServerSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.17566710545920… |
114.16 KiB | removed | -114.16 KiB (-100.00%) |
programs/clickhouse-keeper |
DB::ServerSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.54623650351436… |
new | 114.16 KiB | +114.16 KiB |
programs/clickhouse-keeper |
DB::MergeTreeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.16712493492… |
102.51 KiB | removed | -102.51 KiB (-100.00%) |
programs/clickhouse-keeper |
DB::MergeTreeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.56540966842… |
new | 102.51 KiB | +102.51 KiB |
programs/clickhouse |
DB::MergeTreeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.56540966842… |
new | 102.51 KiB | +102.51 KiB |
programs/clickhouse |
DB::MergeTreeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.16712493492… |
102.51 KiB | removed | -102.51 KiB (-100.00%) |
programs/clickhouse |
DB::DatabaseDataLakeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.1111… |
new | 97.41 KiB | +97.41 KiB |
programs/clickhouse |
DB::DatabaseDataLakeSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.9031… |
97.41 KiB | removed | -97.41 KiB (-100.00%) |
programs/clickhouse |
DB::ObjectStorageQueueSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.38… |
new | 92.15 KiB | +92.15 KiB |
programs/clickhouse |
DB::ObjectStorageQueueSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.26… |
92.15 KiB | removed | -92.15 KiB (-100.00%) |
programs/clickhouse |
DB::KafkaSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.872715978862839… |
new | 89.47 KiB | +89.47 KiB |
programs/clickhouse |
DB::KafkaSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.176316896530569… |
89.47 KiB | removed | -89.47 KiB (-100.00%) |
programs/clickhouse |
DB::DataLakeStorageSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.41967… |
88.38 KiB | removed | -88.38 KiB (-100.00%) |
programs/clickhouse |
DB::DataLakeStorageSettingsTraits::Accessor::instance()::$_0::operator()() const [clone .llvm.13841… |
new | 88.38 KiB | +88.38 KiB |
CI finish ledger - 3c46800Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task CI is fully finished on this head: 175 check-runs, 0 queued or in progress,
Session id: cron:our-pr-ci-monitor:20260803-053000 |
Related: #111418
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix
accurateCast,accurateCastOrNullandaccurateCastOrDefaultfrom an unsigned integer toIPv4, and fromUInt128toIPv6: they never converted the value even though the plainCASTof it succeeds. Because the MergeTree set index builds its pruning set withaccurateCastOrNull, anIPv4primary key withINover aUInt32subquery returned no rows.Description
accurateCastOrNull(x, 'IPv4')returnedNULLfor every unsigned integer,accurateCastthrew, andaccurateCastOrDefaultreturned0.0.0.0- whileCAST(x, 'IPv4')converts it. Same forUInt128toIPv6:FunctionCast::createWrapperenters its accurate branch for any integer source, then dispatches into a lambda whose value-producing arms are guarded byIsDataTypeNumber<RightDataType>. That trait is specialized only forDataTypeNumber<T>, whichDataTypeIPv4/DataTypeIPv6are not: their field type is aStrongTypedef. No arm matched, so the wrapper fell through to an all-NULLcolumn or a throw without examining the value. PlainCASTreachesConvertImpl, which already supports integer toIPv4.This adds the missing dispatcher arm plus an accurate integer-to-
IPv4conversion that range-checks againstIPv4::UnderlyingType, mirroringconvertFieldToType. That check is load-bearing: the existing helper truncates, so reusing it would replaceNULLwith a silently wrong value.UInt128toIPv6needs only the dispatcher arm. Fixing the conversion layer corrects every consumer at once; aKeyCondition-only fix would leave symptoms 2 and 3.Plain
CASTkeeps its behaviour, including truncation above 2^32. Out-of-range values are now rejected rather than truncated, and the set index still prunes (Parts: 1/3, asserted).03212_variant_dynamic_cast_or_defaultis updated becausetoIPv4OrDefault/toIPv6OrDefaultbuild on the accurate cast, as in #110459 forDate32.How the scope was established (702-pair cast sweep)
Consumers corrected by the single change:
Set(runtimeIN),KeyCondition(the set index),castOrDefault,KVStorageUtils,FunctionsJSON,evaluateConstantExpression.All 702 ordered pairs over 27 types were swept against the invariant "plain
CASTsucceeds, so theaccurate cast must not return
NULL". Exactly 6 pairs violated it, and all 6 are fixed here with noresidue:
CASTaccurateCastOrNullbeforeUInt8IPv40.0.0.1NULLUInt16IPv40.0.0.1NULLUInt32IPv40.0.0.1NULLUInt64IPv40.0.0.1NULLBoolIPv40.0.0.1NULLUInt128IPv6::1NULLBoolneeds no special handling, beingDataTypeUInt8.UUIDand theInt*/Float*/UInt256sources stay out of scope: plain
CASTthrows for those too, soNULLis already consistent, andmaking them convertible would be a new capability rather than a bug fix. The new test pins them as
negative controls, together with the
4294967295/4294967296boundary pair and theNullable/LowCardinality/Constwrapper matrix.