Fix setting ignored for numeric-to-DateTime64/Time64 casts#100535
Fix setting ignored for numeric-to-DateTime64/Time64 casts#100535RenzoMXD wants to merge 23 commits into
Conversation
yariks5s
added
the
can be tested
clickhouse-gh
Bot
added
the
pr-bugfix
RenzoMXD
force-pushed
the
fix-datetime-overflow-behavior
branch
from
0c9fc33 to
1d4b032
Compare
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
RenzoMXD
force-pushed
the
fix-datetime-overflow-behavior
branch
from
1d4b032 to
afa6e33
Compare
|
@yariks5s |
|
@rschu1ze @alexey-milovidov @yariks5s |
|
@RenzoMXD I will check your PR soon, there are a lot of PRs to check, so it can sometimes take some time to review pull requests. Anyway, I remember about your pr and will check it.🙂 |
|
@RenzoMXD Thanks for the contribution(s). There is generally no need to ping random people, all PRs get eventually reviewed (it may just take a while). |
|
Hi, @yariks5s. Sorry for interrupting you. |
|
@RenzoMXD In general, it is really not needed to ask for new reviews once a day. This creates a false sense of urgency and priority on our end. |
|
Hi, @yariks5s @alexey-milovidov @rschu1ze |
|
The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix. |
|
@alexey-milovidov Please give me any advice for this failure. |
|
The failures of "Flaky check" in "functions_bad_arguments" will be fixed by #101994. |
…onversions When a UInt64 above INT64_MAX is converted to DateTime64 / Time with date_time_overflow_behavior = saturate or ignore, the previous code clamped via std::min(time_t(from), MAX_*). Casting such a value to signed time_t is implementation-defined and typically wraps to a negative number, so the clamp returned the negative value instead of saturating to the upper bound. Clamp in the unsigned domain first (std::min<UInt64>), then cast, mirroring the pattern already used by ToTime64TransformUnsigned. Also rename the test added in this PR to 04141_* to avoid the 04056_* prefix collision with nine unrelated tests, and add boundary regressions exercising 18446744073709551615::UInt64. Refs ClickHouse#100535 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit's test added two queries:
SELECT toTime(18446744073709551615::UInt64) SETTINGS date_time_overflow_behavior = ...
which fail with `Illegal type UInt64 of argument of function toTimeWithFixedDate`.
The `toTime` function does not accept a numeric input, so these queries are
invalid. Drop them; the matching `toDateTime64(...)` and `CAST(..., 'DateTime64(3)')`
boundary queries still exercise the fix in `ToDateTime64TransformUnsigned`.
Also rename the test from 04141 to 04145: 04141 has been claimed by five
unrelated tests on master since the previous rename.
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100535&sha=7f0d78ba58a7e0d1ea30f709451ae6f8b8ca6ca5&name_0=PR&name_1=Fast%20test
PR: ClickHouse#100535
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hello! Why this PR was closed? |
| { | ||
| if constexpr (std::is_same_v<ToDataType, DataTypeDateTime>) | ||
| return DateTimeTransformImpl<FromDataType, ToDataType, ToDateTimeTransform64<typename FromDataType::FieldType, UInt32, default_date_time_overflow_behavior>, false>::template execute<Additions>( | ||
| return DateTimeTransformImpl<FromDataType, ToDataType, ToDateTimeTransform64<typename FromDataType::FieldType, UInt32, date_time_overflow_behavior>, false>::template execute<Additions>( |
There was a problem hiding this comment.
Routing UInt64 -> DateTime through date_time_overflow_behavior here exposes the same signed-intermediate bug that this PR already had to fix for Time and DateTime64.
ToDateTimeTransform64::execute still clamps with std::min(time_t(from), time_t(MAX_DATETIME_TIMESTAMP)), so values above INT64_MAX wrap before the clamp. On a typical 64-bit signed time_t, CAST(9223372036854775808::UInt64, 'DateTime') SETTINGS date_time_overflow_behavior = 'saturate' goes through INT64_MIN, then casts back to UInt32 as 0, i.e. 1970-01-01 00:00:00 instead of saturating to 2106-02-07 06:28:15.
The same pattern is still present in the UInt64 -> Date / Date32 branches that were switched above: both helpers convert the huge unsigned value to a signed time_t / Int64 before applying std::min, so saturate / ignore can wrap toward the low end instead of the maximum representable date.
Please clamp in the unsigned domain first (same pattern as ToTimeTransform64 / ToDateTime64TransformUnsigned) and add regressions for at least CAST(9223372036854775808::UInt64, 'DateTime') and CAST(9223372036854775808::UInt64, 'Date') with date_time_overflow_behavior = 'saturate'.
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered: 49/62 (79.03%) · Uncovered code |
5 participants
Fix
date_time_overflow_behaviorsetting being ignored when casting numeric types (Int64, UInt64, Float64) toDateTime64/Time64. The setting was silently clamping values instead of throwingVALUE_IS_OUT_OF_RANGE_OF_DATA_TYPEwhen set tothrow.Two root causes:
In
ConvertImpl::execute(FunctionsConversion.h), 15 conversion branches passed the hardcodeddefault_date_time_overflow_behavior(Ignore) instead of the template parameterdate_time_overflow_behaviorto the transform structs. This broke thetoDateTime64/toTime64runtime path.In
FunctionCast::createDecimalWrapper(FunctionsConversion.cpp),ConvertImplwas called without specifying the overflow behavior template parameter, always defaulting toIgnore. This broke theCAST(... AS DateTime64)path, including constant-folded expressions.Fixes #100471
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix
date_time_overflow_behaviorsetting being ignored for numeric-to-DateTime64/Time64conversions viaCAST,toDateTime64, andtoTime64.Documentation entry for user-facing changes
No documentation changes needed — this is a bug fix for existing documented behavior.