Skip to content

fix: DateTime64BestEffort milli/nano precision#105233

Merged
scanhex12 merged 3 commits into
ClickHouse:masterfrom
kavirajk:kavirajk/fix-datetime-best-effort-milli-nano-seconds
May 19, 2026
Merged

fix: DateTime64BestEffort milli/nano precision#105233
scanhex12 merged 3 commits into
ClickHouse:masterfrom
kavirajk:kavirajk/fix-datetime-best-effort-milli-nano-seconds

Conversation

@kavirajk
Copy link
Copy Markdown
Contributor

@kavirajk kavirajk commented May 18, 2026

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix best_effort date time parsing work correctly with toDateTime64 with ms and ns precision.

Explanation.

Looks like enabling the "best_effort" as default recently caused this?

This is exposed by the golang-client tests that runs on head of the master branch.. It used to work in previous versions correctly (because it was using basic as default)

Before:

$ clickhouse-local
ClickHouse local version 26.3.9.8 (official build).

:) SELECT toDateTime64('1779094968417585845', 9) SETTINGS  cast_string_to_date_time_mode='best_effort'

SELECT toDateTime64('1779094968417585845', 9)
SETTINGS cast_string_to_date_time_mode = 'best_effort'

Query id: b390c272-5a88-4b58-ab93-4b80429e0c1d


Elapsed: 0.014 sec.

Received exception:
Code: 41. DB::Exception: Cannot read DateTime: unexpected number of decimal digits: 19: Cannot parse DateTime64(9) from String: In scope SELECT toDateTime64('1779094968417585845', 9) SETTINGS cast_string_to_date_time_mode = 'best_effort'. (CANNOT_PARSE_DATETIME)

After:

:) SELECT toDateTime64('1779094968417585845', 9) SETTINGS  cast_string_to_date_time_mode='best_effort'

SELECT toDateTime64('1779094968417585845', 9)
SETTINGS cast_string_to_date_time_mode = 'best_effort'

Query id: d11d292d-6183-4b2d-8bff-1af10e9bee08

   ┌─toDateTime64('⋯417585845', 9)─┐
1. │ 2026-05-18 11:02:48.417585845 │
   └───────────────────────────────┘

1 row in set. Elapsed: 0.004 sec.

Looks like enabling the "best_effort" as default recently caused this?

Before:
```
:) SELECT toDateTime64('1779094968417585845', 9) SETTINGS  cast_string_to_date_time_mode='besteffort'

SELECT toDateTime64('1779094968417585845', 9)
SETTINGS cast_string_to_date_time_mode = 'besteffort'

Query id: f3b9bc78-9ec3-46f7-bbf3-65795a61b5ec

Received exception:
Code: 36. DB::Exception: Unexpected value of DateTimeInputFormat: 'besteffort'. Must be one of ['best_effort_us', 'best_effort', 'basic']. (BAD_ARGUMENTS) (version 26.5.1.740 (official build))
```

After:
```
:) SELECT toDateTime64('1779094968417585845', 9) SETTINGS  cast_string_to_date_time_mode='best_effort'

SELECT toDateTime64('1779094968417585845', 9)
SETTINGS cast_string_to_date_time_mode = 'best_effort'

Query id: d11d292d-6183-4b2d-8bff-1af10e9bee08

   ┌─toDateTime64('⋯417585845', 9)─┐
1. │ 2026-05-18 11:02:48.417585845 │
   └───────────────────────────────┘

1 row in set. Elapsed: 0.004 sec.

```
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented May 18, 2026

Workflow [PR], commit [39982a1]

Summary:


AI Review

Summary

This PR extends best_effort numeric epoch parsing in parseDateTimeBestEffortImpl to cover 16-digit microsecond and 19-digit nanosecond inputs (in addition to existing 13-digit millisecond handling), and adds focused stateless regression tests for both explicit cast_string_to_date_time_mode = 'best_effort' and inherited-default behavior. The previously discussed ambiguity around pre-2001 subsecond timestamps is now explicitly documented and pinned by tests.

Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label May 18, 2026
Comment thread src/IO/parseDateTimeBestEffort.cpp
@scanhex12 scanhex12 assigned scanhex12 and unassigned scanhex12 May 18, 2026
@antaljanosbenjamin
Copy link
Copy Markdown
Member

It is a bit unclear for me, what is the actual bug. For me, it looks like an improvement than a fix. In the "before" example the only issue is besteffort was written instead of best_effort. The docs don't mention unix timestamps:

ClickHouse can parse the basic YYYY-MM-DD HH:MM:SS format and all ISO 8601 date and time formats

I think the PR is valuable and really nice addition, but I wouldn't call this as a bugfix.

The gap is real. Pre 09-09-2001, unix timestamp was 9 digit.
But this already a problem with existing branches (e.g: 13 ms branch on the same function).

Also in the src/IO/ReadHelpers.h this behavior and shortcoming is explicitly documented.

Even if we want to fix this behavior correctly, we need scale (seconds = value / 10^scale)
to splitting from the right (3, 6 or 9).
but parseDateTimeBestEffortImpl doesn't currently see scale and it's unaware of it.
Fixing this correctly might need bigger refactoring I believe.

Instead I added comments to make this behavior explicit on this function
and added tests to lock this behavior to avoid future surprises

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk
Copy link
Copy Markdown
Contributor Author

kavirajk commented May 18, 2026

@antaljanosbenjamin sorry for the confusion. Updated the PR description.

It is indeed a bug. You can reproduce the bug even without that best_effort settings (because that is default now)

You can see the difference in behavior simply running the queries without the settings.

Before (in v26.3.9.8) - It works

$ clickhouse-local
ClickHouse local version 26.3.9.8 (official build).

:) SELECT toDateTime64('1779094968417585845', 9)

SELECT toDateTime64('1779094968417585845', 9)

Query id: ea71dba1-ddbd-44a6-be99-169e295f182b

   ┌─toDateTime64('⋯417585845', 9)─┐
1. │ 2026-05-18 11:02:48.417585845 │
   └───────────────────────────────┘

1 row in set. Elapsed: 0.003 sec.

Before (in current master) - It breaks

$ clickhouse-master local
ClickHouse local version 26.5.1.740 (official build).

:) SELECT toDateTime64('1779094968417585845', 9)

SELECT toDateTime64('1779094968417585845', 9)

Query id: 97c24edf-a974-462e-9fcb-40fb7c14fef4


Elapsed: 0.061 sec.

Received exception:
Code: 41. DB::Exception: Cannot read DateTime: unexpected number of decimal digits: 19: Cannot parse DateTime64(9) from String: In scope SELECT toDateTime64('1779094968417585845', 9). (CANNOT_PARSE_DATETIME)

:)

After (this PR) - works

$ ./build/programs/clickhouse-local
ClickHouse local version 26.5.1.1.

:) SELECT toDateTime64('1779094968417585845', 9)

SELECT toDateTime64('1779094968417585845', 9)

Query id: a2566568-bfba-4edd-99b4-711197c56643

   ┌─toDateTime64('⋯417585845', 9)─┐
1. │ 2026-05-18 11:02:48.417585845 │
   └───────────────────────────────┘

1 row in set. Elapsed: 0.003 sec.

:)


@antaljanosbenjamin
Copy link
Copy Markdown
Member

You can see the difference in behavior simply running the queries without the settings.

Ah, okay, now it is clear. Thanks for clarifying.

Added tests to lock the behavior of the explicit "best_effort" and
default behavior to avoid future regression

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented May 19, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.00% +0.00%
Functions 91.40% 91.40% +0.00%
Branches 76.50% 76.50% +0.00%

Changed lines: 83.33% (15/18) | lost baseline coverage: 4 line(s) · Uncovered code

Full report · Diff report

@scanhex12 scanhex12 added this pull request to the merge queue May 19, 2026
Merged via the queue into ClickHouse:master with commit 7a9ee85 May 19, 2026
165 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants