Workflow [PR], commit [50f8f1a]

Summary: ❌

AI Review

Summary

This PR fixes date_time_overflow_behavior handling for integer and Float32/Float64 casts to DateTime64 and Time64, including wide integers, scale-aware extrema, NaN/infinity handling, and boundary-fraction cases. The current conversion code matches the narrowed PR contract, but the generated setting documentation is stale for the behavior now implemented.

Findings

⚠️ Majors

• [src/Functions/FunctionsConversion.h:2260] The source-level documentation for date_time_overflow_behavior must describe the same source and target surface that the code implements. This branch now applies the setting to Float32/Float64 casts into DateTime64/Time64, and the integer branches apply it to Time64, but src/Core/FormatFactorySettings.h still describes only Date/Date32/DateTime/DateTime64 conversions and integer inputs. Users reading the generated setting docs will miss both the float sources and the Time64 target.
  Suggested fix: update the setting description to include integer/float sources and Time/Time64 targets.

Final Verdict

Status: ⚠️ Request changes

Minimum required action: update the date_time_overflow_behavior setting documentation in src/Core/FormatFactorySettings.h to match the expanded DateTime64/Time64 cast behavior.

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.

The failures of "Flaky check" in "functions_bad_arguments" will be fixed by #101994.

This was fixed by #105146. Let's update the branch.

Picked up the branch: merged latest master (picks up the fixes you flagged — #101239, #101994, #105146) and addressed the two red CI jobs plus the open DateTime64(9) review blocker.

Build (arm_tidy) — variable 'clamped' is not initialized (cppcoreguidelines-init-variables). Initialized clamped in the signed DateTime64/Time64 transforms.

Fast test / 03011_definitive_guide_to_cast and the scale-9 blocker turned out to be the same root cause and are fixed together in f6a9c2d:

• The overflow bounds were computed from the calendar maximum second (MAX_DATETIME64_TIMESTAMP), but at scale 9 that second does not fit in the Int64 native storage (10413791999 * 1e9 > Int64::max). So a value could pass the bound check and then raise DECIMAL_OVERFLOW inside the conversion even under date_time_overflow_behavior = 'saturate', where it must clamp.
• The bounds are now derived from the maximum representable native value, capped at Int64::max, in the float and the signed/unsigned integer transforms (the integer paths had the same scale-9 failure mode). saturate clamps to the maximum representable value and throw raises VALUE_IS_OUT_OF_RANGE_OF_DATA_TYPE.
• The float path now produces saturated values directly in the native domain, so CAST(1e20 AS DateTime64(6)) yields the exact 2299-12-31 23:59:59.999999 instead of the float-imprecise ...999998 that was breaking 03011.

I updated the 03011 comment/reference for the DateTime64(9) case (it now saturates instead of throwing DECIMAL_OVERFLOW — matching your note that this behavior was fine to change) and added throw/saturate regression coverage for DateTime64(9) (Float64/UInt64/Int64 sources) to 04050_time64_cast_uint64_no_wrap.

Built on ARM and verified locally: 03011 and 04050 match their references, and a sweep of 13 related date/time cast tests passes. The Decimal*/BFloat16 thread remains as you dismissed it (scale-aware handling belongs in DataTypesDecimal.cpp, out of scope here).

I did not approve, since this is not my own PR.

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 38 queries analysed

This PR changes only the numeric-to-DateTime64/Time64 CAST overflow logic (clamping, saturate/throw behavior, and wide-integer support) in the conversion functions, plus the accompanying tests. None of the four flagged ClickBench queries (Q4, Q15, Q28, Q33) cast numbers into DateTime64 or Time64, so the modified code path is never executed by them and cannot plausibly produce these improvements. All four flagged improvements are downgraded to not-sure as run-to-run variance rather than real PR effects. No genuine cross-query performance pattern is present.

clickbench

⚠️ 4 inconclusive

tpch_adapted_1_official

🟢 No significant changes

Updated the branch: merged latest master (232 commits, conflict-free) and finished the two open Float32 review blockers on the float overflow path (d3e60548107).

The ToDateTime64TransformFloat / ToTime64TransformFloat transforms still decided overflow from a bound narrowed back to the source float type (max_native / scale_multiplier in FromType), which for Float32 rounds to the wrong side — letting an out-of-range value pass the throw check or clamping an in-range one. The decision now compares the whole-second part as an integer and the sub-second part in the Float64 domain (exact enough at every scale, deterministic across architectures).

The in-range conversion is also now done in the Float64 domain rather than via convertToDecimal, whose range check runs in the source-float precision: a Float32 value just below the DateTime64(9) maximum is indistinguishable from Int64::max at Float32 precision, so convertToDecimal would spuriously raise DECIMAL_OVERFLOW. Float64 reproduces convertToDecimal exactly for Float64 sources and is strictly more faithful for Float32 ones, so CAST(9223371776::Float32, 'DateTime64(9, ''UTC'')) is now preserved as 2262-04-11 23:42:56.000000000.

Built on this worktree and verified locally: 01734, 03011, 04050 match their references, and a sweep of 78 datetime64/time64 cast tests passes (the only non-passing two are infra-only — user_files permissions and a remote('127.0.0.{1,2}', ...) cluster). Added Float32 regression coverage to 04050_time64_cast_uint64_no_wrap and completed the 01734 reference for the DateTime64(9) saturation cases.

The Decimal*/BFloat16 thread remains as previously dismissed (out of scope here).

Pushed updates:

• dff3f8ab592 — updated the 02462_number_to_datetype reference: toDateTime64(toFloat32(...)) now scales in the Float64 domain, so the result is the exact value of the Float32 input rather than a value distorted by source-float-precision scaling. This fixes the sole Fast test failure.
• 919a29605c3 — addressed the two remaining review findings on the float transforms: NaN is rejected (throw) or clamped (saturate) instead of reaching an undefined static_cast, and the overflow decision is now made on the truncated scaled-native value the conversion actually stores, so valid boundary-fraction inputs like 10413791999.999::Float64 for DateTime64(3) are no longer falsely rejected. Both with tests in 04050_time64_cast_uint64_no_wrap.sql.

Verified locally: 04050_time64_cast_uint64_no_wrap, 02462_number_to_datetype, 01734_datetime64_from_float, 03011_definitive_guide_to_cast, 02900_date_time_check_overflow, 03271_date_to_datetime_saturation, 04092_date_plus_time, 02373_datetime64_monotonicity, 00921_datetime64_compatibility_long all pass against this branch.

The remaining unresolved thread (Decimal*/BFloat16 sources) is deferred to a follow-up as discussed above.

Pushed 1a136704eec (and merged latest master) addressing the remaining AI-review blocker on the integer saturate clamp.

Integer saturate clamp now reaches the exact target-type boundary. The four integer transforms (ToDateTime64TransformUnsigned/Signed, ToTime64TransformUnsigned/Signed) clamped an out-of-range value to the whole-second maximum, dropping the sub-second fraction, while the float transforms clamped to the maximum representable native value. They now clamp saturate to maxRepresentableDateTime64Native / maxRepresentableTime64Native, so the integer and float results agree at every scale:

• CAST(toUInt64(10413791999), 'DateTime64(9, 'UTC')) with date_time_overflow_behavior='saturate' → 2262-04-11 23:47:16.854775807 (was …16.000000000); same for the signed Int64 source.
• CAST(toUInt64(99999999999), 'DateTime64(3, 'UTC')) saturate → 2299-12-31 23:59:59.999 (was …59.000).
• CAST(18446744073709551615::UInt64, 'Time64') saturate → 999:59:59.999; negative overflow clamps to -999:59:59.999.

The legacy whole-second clamp is kept for ignore only, as suggested in the review (so the default behavior is unchanged — 01734 and other default-behavior references are untouched).

04050_time64_cast_uint64_no_wrap is updated: the saturate comparisons now assert that an out-of-range integer cast equals the corresponding out-of-range float cast, and the DateTime64(9) / Time64 direct-value references reflect the max representable value. Verified locally — 04050, 01734, 03011, 02462, 02900_date_time_check_overflow, 03271_date_to_datetime_saturation, 02373_datetime64_monotonicity all pass against the rebuilt binary.

The Decimal* / BFloat16 source thread remains deferred to a follow-up as previously discussed (scale-aware overflow handling for those families belongs in DataTypesDecimal.cpp, not in this header); the changelog scope is limited accordingly.

Addressed the remaining AI-review "Request changes" verdict (the Decimal* / BFloat16 source-family gap) via its own suggested remedy — narrowing the PR title and changelog to the families actually fixed here, rather than expanding scope.

• Title narrowed from "… for DateTime64/Time64 numeric casts" to "… for integer and float casts to DateTime64/Time64" (the title was also malformed — it ended in a stray … whose tail had spilled into the PR body's first line; that is now cleaned up).

• Changelog entry rewritten to scope the fix to integer and Float32/Float64 sources and to mention both saturate clamping and the Float32 precision change, matching the AI's suggested replacement text:

  Fixes date_time_overflow_behavior for integer and Float32/Float64 casts to DateTime64 and Time64: overflowing values now throw VALUE_IS_OUT_OF_RANGE_OF_DATA_TYPE in throw mode and clamp to the target boundary in saturate mode; Float32 inputs are scaled in Float64 precision, correcting previously imprecise results.

Decimal* / BFloat16 sources remain deferred to a follow-up as discussed — their scale-aware overflow handling belongs in DataTypesDecimal.cpp, not in FunctionsConversion.h. The corresponding review thread is resolved since the AI's stated condition (narrow the title/changelog) is now met.

No code change and no re-merge: the head is MERGEABLE, CI is green on f1fa314a, and the branch merged master ~2 days ago (well under a week), so a fresh master merge is not warranted.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 98/107 (91.59%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 2 line(s) · Uncovered code

Full report · Diff report