Saturate date/datetime to zero. by amosbird · Pull Request #29953 · ClickHouse/ClickHouse

amosbird · 2021-10-10T14:36:56Z

Changelog category (leave one):

Improvement

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

Now date time conversion functions that generates time before 1970-01-01 00:00:00 will be saturated to zero instead of overflow.

#2327
#3470
#12377
#17354
#18511
#22928
#25284
#26137
#27150
#29835

Detailed description / Documentation draft:
.

alexey-milovidov · 2021-10-10T15:10:10Z

src/Functions/DateTimeTransforms.h

This is strange.

We have a comment says that 1970-01-01 is used as the date part: https://github.com/ClickHouse/ClickHouse/blob/c6e02a534f97720471e67c5d9cfddbc0fef62b1f/src/Functions/DateTimeTransforms.h#L239

However the old behavior apparently uses 1970-01-02 which most likely to avoid overflow. After this PR it's ok.

I recommend not changing toTime as it has more chance to break some use cases.

Ok. I'll change the comment instead.

alexey-milovidov · 2021-10-10T15:10:42Z

tests/queries/0_stateless/02096_date_time_1970_saturation.sql

Check for western time zones, like America/Los_Angeles.

qoega · 2021-10-10T16:02:05Z

Is it pr-backward-incompatible?

alexey-milovidov · 2021-10-10T16:09:58Z

Overflow has implementation defined behaviour in ClickHouse, and this behaviour is changed.
You can say that the change is backward compatible, although some users may rely on the specific behaviour on overflow.

alexey-milovidov · 2021-10-13T22:56:53Z

Perf test is showing a small slowdown.

amosbird · 2021-10-14T08:07:49Z

Perf test is showing a small slowdown.

Maybe we can build another lookup table in DateTimeLUT for this purpose. Not sure if it's appropriate.

alexey-milovidov · 2021-10-23T23:52:51Z

Maybe we can optimize something to compensate this slowdown...

amosbird · 2021-11-01T14:18:22Z

tests/queries/0_stateless/01213_alter_rename_with_default_zookeeper_long.sql

@alesapin Could you explain why we choose 10000 month for TTL? It leads to implementation defined date overflow.

alexey-milovidov · 2021-11-18T02:19:19Z

There is one warning from gcc.

amosbird · 2021-11-18T15:13:19Z

There is one warning from gcc.

Fixed.

alexey-milovidov · 2021-11-18T19:21:11Z

7% performance decrease: https://clickhouse-test-reports.s3.yandex.net/29953/903df9df6b44d9b31e2992c513633713da0bfadc/performance_comparison/report.html#fail1

Maybe it's ok to merge?

amosbird · 2021-11-19T08:20:53Z

7% performance decrease: https://clickhouse-test-reports.s3.yandex.net/29953/903df9df6b44d9b31e2992c513633713da0bfadc/performance_comparison/report.html#fail1

Maybe it's ok to merge?

It's interesting that my local clickhouse benchmark comparison shows the opposite result:

tests/performance/parallel_index.xml:


This PR, queries 22, QPS: 0.896, RPS: 939902.487, MiB/s: 7.171, result RPS: 0.896, result MiB/s: 0.000.
Upstream, queries 24, QPS: 0.732, RPS: 712294.468, MiB/s: 5.434, result RPS: 0.732, result MiB/s: 0.000.

0.000%          0.973 sec.      1.205 sec.
10.000%         1.025 sec.      1.245 sec.
20.000%         1.030 sec.      1.288 sec.
30.000%         1.053 sec.      1.297 sec.
40.000%         1.093 sec.      1.351 sec.
50.000%         1.097 sec.      1.362 sec.
60.000%         1.110 sec.      1.386 sec.
70.000%         1.175 sec.      1.405 sec.
80.000%         1.197 sec.      1.429 sec.
90.000%         1.237 sec.      1.503 sec.
95.000%         1.265 sec.      1.522 sec.
99.000%         1.284 sec.      1.560 sec.
99.900%         1.284 sec.      1.560 sec.
99.990%         1.284 sec.      1.560 sec.

Difference at 99.5% confidence: mean difference is 0.25035227, but confidence interval is 0.08870240

alexey-milovidov · 2022-01-03T16:44:29Z

The changes in performance look insignificant. Ok to merge.

alexey-milovidov · 2022-01-03T16:45:19Z

@amosbird Please close the issues that were fixed.

amosbird · 2022-01-04T03:13:05Z

Only one issue left #25284 .

azat · 2022-02-07T08:11:24Z

tests/queries/0_stateless/02096_date_time_1970_saturation.sql

+select toStartOfQuarter(toDateTime(0, 'America/Los_Angeles'));
+select toStartOfYear(toDateTime(0, 'America/Los_Angeles'));
+select toTime(toDateTime(0, 'America/Los_Angeles'), 'America/Los_Angeles');
+select toStartOfMinute(toDateTime(0, 'America/Los_Angeles'));


@amosbird Any reason that you decided not to saturate toStartOfMinute and friends?

It should be.

┌─toStartOfMinute(toDateTime(0, 'America/Los_Angeles'))─┐ │ 1969-12-31 16:00:00 │ └───────────────────────────────────────────────────────┘

Actually it does not always work.

toStartOfHour

Does not work for partial hours timezones, i.e. America/Paramaribo

┌─toStartOfHour(toDateTime(0, 'America/Paramaribo'))─┐ │ 2106-02-07 02:58:16 │ └────────────────────────────────────────────────────┘

toStartOfMinute

Does not work for partial minutes timezones, Africa/Monrovia - which is very special timezone, #27080 for the reference.

$ TZ=Africa/Monrovia date -d@1 Wed Dec 31 11:15:31 PM MMT 1969 ^^

┌─toStartOfMinute(toDateTime(0, 'Africa/Monrovia'))─┐ │ 2106-02-07 06:27:46 │ └───────────────────────────────────────────────────┘

And this breaks monotonicity of the function.

It can be fixed to avoid breaking monotonicity, but this will make the result of functions incorrect and also breaks backward compatibility for negative timestamps.

Also note, that it had been changed already in 21.4 select toDateTime(1, 'Africa/Monrovia') dt, toStartOfMinute(dt):

21.3: 1970-01-01 00:43:01 1970-01-01 00:43:00

21.4: 1969-12-31 23:15:31 1969-12-31 23:15:30 Date time64 extended range #9404

… flakiness It is possible for toStartOfMinute() to give different result for 0 and 59, for partial timezones (timezone that does not starts from 00:00, like Africa/Monrovia). Before ClickHouse#36656 it fails for another reason, because of overflows [1], but now it fails because it simply return different minutes. [1]: ClickHouse#29953 (comment) Simply pin the UTC there. Fixes: ClickHouse#37786 Signed-off-by: Azat Khuzhin <[email protected]>

robot-clickhouse added the pr-improvement Pull request with some product improvements label Oct 10, 2021

alexey-milovidov reviewed Oct 10, 2021

View reviewed changes

alexey-milovidov self-assigned this Oct 10, 2021

amosbird force-pushed the saturatetime branch 2 times, most recently from b413a93 to 9e0d0ae Compare October 11, 2021 09:29

amosbird force-pushed the saturatetime branch 3 times, most recently from 027a2a7 to 38b1e2f Compare November 1, 2021 10:35

amosbird commented Nov 1, 2021

View reviewed changes

amosbird force-pushed the saturatetime branch from 17a22c5 to 70344d5 Compare November 8, 2021 07:19

alexey-milovidov approved these changes Nov 17, 2021

View reviewed changes

amosbird force-pushed the saturatetime branch from d024387 to 903df9d Compare November 18, 2021 15:13

alexey-milovidov mentioned this pull request Dec 20, 2021

Inconsistent datetime types converted from string when overflow #32833

Closed

amosbird requested a review from a team as a code owner January 2, 2022 23:06

amosbird and others added 2 commits January 3, 2022 02:07

Saturate date/datetime to zero

0b7a94c

Merge with master

cab5f4f

alexey-milovidov force-pushed the saturatetime branch from 006bef2 to cab5f4f Compare January 2, 2022 23:07

alexey-milovidov merged commit cb19d6f into ClickHouse:master Jan 3, 2022

amosbird mentioned this pull request Jan 4, 2022

Avoid wraparound (timestamp underflow) in toStartOfWeek() #26137

Closed

alexey-milovidov mentioned this pull request Jan 13, 2022

Wrong result for query with minimal timestamp 1970-01-01 00:00:00 #33592

Closed

azat reviewed Feb 7, 2022

View reviewed changes

amosbird mentioned this pull request Apr 26, 2022

Saturate date/datetime to zero (part 2) #36656

Merged

azat mentioned this pull request Aug 10, 2022

tests: fix 00926_adaptive_index_granularity_pk/00489_pk_subexpression flakiness #40075

Merged

Conversation

amosbird commented Oct 10, 2021

Uh oh!

alexey-milovidov Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

amosbird Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amosbird Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

qoega commented Oct 10, 2021

Uh oh!

alexey-milovidov commented Oct 10, 2021

Uh oh!

alexey-milovidov commented Oct 13, 2021

Uh oh!

amosbird commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Oct 23, 2021

Uh oh!

amosbird Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Nov 18, 2021

Uh oh!

amosbird commented Nov 18, 2021

Uh oh!

alexey-milovidov commented Nov 18, 2021

Uh oh!

amosbird commented Nov 19, 2021

Uh oh!

alexey-milovidov commented Jan 3, 2022

Uh oh!

alexey-milovidov commented Jan 3, 2022

Uh oh!

amosbird commented Jan 4, 2022

Uh oh!

azat Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

amosbird Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

azat Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

toStartOfHour

toStartOfMinute

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexey-milovidov Oct 11, 2021 •

edited

Loading

amosbird commented Oct 14, 2021 •

edited

Loading

azat Feb 7, 2022 •

edited

Loading

`toStartOfHour`

`toStartOfMinute`