Add time bucket function #5665

papparapa · 2022-12-12T12:21:15Z

Edit: PR of non-ICU time_bucket function has been split into #5835.

Implemented four overloads of ICU time_bucket function.
They behave almost same as TimescaleDB's time_bucket.

TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMP)
TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMPTZ)
TIME_BUCKET(bucket_width: INTERVAL, ts: DATE)

TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMP, offset: INTERVAL)
TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMPTZ, offset: INTERVAL)
TIME_BUCKET(bucket_width: INTERVAL, ts: DATE, offset: INTERVAL)

TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMP, origin: TIMESTAMP)
TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMPTZ, origin: TIMESTAMPTZ)
TIME_BUCKET(bucket_width: INTERVAL, ts: DATE, origin: DATE)

TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMPTZ, timezone: TEXT)

To ease the tests, I set the default origin to '2000-01-03 00:00:00+00' (when bucket_width is small) or '2000-01-01 00:00:00+00' (when bucket_width is large) as the same as TimescaleDB.

hawkfish

It might be a good idea to remove the TIMESTAMPTZ versions from this PR and implement them separately. The implementations you have now can be accessed by just casting the TSTZ values to TS.

src/common/types/date.cpp

src/include/duckdb/common/types/date.hpp

test/sql/function/date/test_time_bucket_date.test

test/sql/function/date/test_time_bucket_timestamptz.test

test/sql/function/date/test_time_bucket_timestamp.test

src/function/scalar/date/time_bucket.cpp

papparapa · 2022-12-12T23:42:13Z

Thanks @hawkfish ! I will fix them.

papparapa · 2022-12-18T07:26:55Z

Added one more overload

TIME_BUCKET(bucket_width: INTERVAL, ts: TIMESTAMPTZ, timezone: TEXT)

hawkfish

Thanks for doing this - especially the TimebaseDB compatibility. And I'm sorry someone posted that this is a "Good First Issue"! It's easy enough for instants (and you did a great job with that), but time zones are a black art.

In particular, "local timestamps" are a dangerous hack and should be avoided whenever possible. We only have support for them because some people have dirty data that uses them and this lets them (mostly) clean them up, and also for compatibility with Postgres and the SQL standard.

I may have mentioned my blog post on time binning, but not you might want to read it if I am not making sense.

extension/icu/icu-timebucket.cpp

src/include/duckdb/common/operator/convert_to_string.hpp

src/include/duckdb/common/types/interval.hpp

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test

test/sql/function/timestamp/test_time_bucket_timestamp.test

hawkfish · 2022-12-19T19:09:29Z

Also I can't resolve the conversations that you have fixed, so can you close them so we can focus on the remaining issues? Thx.

hawkfish

Sorry wanted to ask some more questions!

extension/icu/icu-timebucket.cpp

src/function/scalar/date/time_bucket.cpp

papparapa · 2022-12-20T14:32:32Z

Thanks @hawkfish for careful review!
I'll start with easy fixes.

extension/icu/icu-dateadd.cpp

src/function/scalar/date/time_bucket.cpp

extension/icu/icu-timebucket.cpp

hawkfish · 2022-12-23T18:32:35Z

Thanks for all the hard work! This is coming along nicely, so please don't be discouraged. I'd highly recommend merging from master before your next push - there have been some broken CI bits. Oh and thanks for fixing the formatting - I swear I thought it was included!

src/function/scalar/date/time_bucket.cpp

hawkfish

Getting close! I found some small bugs in the naïve timestamp handling but I don't think we will collide for now.

extension/icu/include/icu-datefunc.hpp

extension/icu/icu-timezone.cpp

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test

papparapa · 2023-01-03T14:09:40Z

The current implementation still has bug-like behavior.

time_bucket result of timestamp on the day DST ended gets greater than the original timestamp.
This is unexpected.

In Europe/London timezone, DST in 2000 started at '2000-03-26 01:00:00+00' and ended at '2000-10-29 02:00:00+01'.

D set timezone = 'Europe/London';
D with t2 as (select t1.ts::timestamptz as ts, time_bucket('10 seconds'::interval, t1.ts::timestamptz, 'Europe/London') as tb from generate_series('2000-10-28 00:00:00'::timestamp, '2000-10-31 00:00:00'::timestamp, '3 hours'::interval) t1 (ts)) select ts, tb, ts = tb as always_should_be_true from t2;
┌──────────────────────────┬──────────────────────────┬───────────────────────┐
│            ts            │            tb            │ always_should_be_true │
│ timestamp with time zone │ timestamp with time zone │        boolean        │
├──────────────────────────┼──────────────────────────┼───────────────────────┤
│ 2000-10-28 00:00:00+01   │ 2000-10-28 00:00:00+01   │ true                  │
│ 2000-10-28 03:00:00+01   │ 2000-10-28 03:00:00+01   │ true                  │
│ 2000-10-28 06:00:00+01   │ 2000-10-28 06:00:00+01   │ true                  │
│ 2000-10-28 09:00:00+01   │ 2000-10-28 09:00:00+01   │ true                  │
│ 2000-10-28 12:00:00+01   │ 2000-10-28 12:00:00+01   │ true                  │
│ 2000-10-28 15:00:00+01   │ 2000-10-28 15:00:00+01   │ true                  │
│ 2000-10-28 18:00:00+01   │ 2000-10-28 18:00:00+01   │ true                  │
│ 2000-10-28 21:00:00+01   │ 2000-10-28 21:00:00+01   │ true                  │
│ 2000-10-29 00:00:00+01   │ 2000-10-29 00:00:00+01   │ true                  │
│ 2000-10-29 03:00:00+00   │ 2000-10-29 04:00:00+00   │ false                 │
│ 2000-10-29 06:00:00+00   │ 2000-10-29 07:00:00+00   │ false                 │
│ 2000-10-29 09:00:00+00   │ 2000-10-29 10:00:00+00   │ false                 │
│ 2000-10-29 12:00:00+00   │ 2000-10-29 13:00:00+00   │ false                 │
│ 2000-10-29 15:00:00+00   │ 2000-10-29 16:00:00+00   │ false                 │
│ 2000-10-29 18:00:00+00   │ 2000-10-29 19:00:00+00   │ false                 │
│ 2000-10-29 21:00:00+00   │ 2000-10-29 22:00:00+00   │ false                 │
│ 2000-10-30 00:00:00+00   │ 2000-10-30 00:00:00+00   │ true                  │
│ 2000-10-30 03:00:00+00   │ 2000-10-30 03:00:00+00   │ true                  │
│ 2000-10-30 06:00:00+00   │ 2000-10-30 06:00:00+00   │ true                  │
│ 2000-10-30 09:00:00+00   │ 2000-10-30 09:00:00+00   │ true                  │
│ 2000-10-30 12:00:00+00   │ 2000-10-30 12:00:00+00   │ true                  │
│ 2000-10-30 15:00:00+00   │ 2000-10-30 15:00:00+00   │ true                  │
│ 2000-10-30 18:00:00+00   │ 2000-10-30 18:00:00+00   │ true                  │
│ 2000-10-30 21:00:00+00   │ 2000-10-30 21:00:00+00   │ true                  │
│ 2000-10-31 00:00:00+00   │ 2000-10-31 00:00:00+00   │ true                  │
├──────────────────────────┴──────────────────────────┴───────────────────────┤
│ 25 rows                                                           3 columns │
└─────────────────────────────────────────────────────────────────────────────┘

On TimescaleDB, this query returns expected result.

postgres=# set timezone = 'Europe/London';
SET
postgres=# with t2 as (select t1.ts::timestamptz as ts, time_bucket('10 seconds'::interval, t1.ts::timestamptz, 'Europe/London') as tb from generate_series('2000-10-28 00:00:00'::timestamp, '2000-10-31 00:00:00'::timestamp, '3 hours'::interval) t1 (ts)) select ts, tb, ts = tb as always_should_be_true from t2;
           ts           |           tb           | always_should_be_true
------------------------+------------------------+-----------------------
 2000-10-28 00:00:00+01 | 2000-10-28 00:00:00+01 | t
 2000-10-28 03:00:00+01 | 2000-10-28 03:00:00+01 | t
 2000-10-28 06:00:00+01 | 2000-10-28 06:00:00+01 | t
 2000-10-28 09:00:00+01 | 2000-10-28 09:00:00+01 | t
 2000-10-28 12:00:00+01 | 2000-10-28 12:00:00+01 | t
 2000-10-28 15:00:00+01 | 2000-10-28 15:00:00+01 | t
 2000-10-28 18:00:00+01 | 2000-10-28 18:00:00+01 | t
 2000-10-28 21:00:00+01 | 2000-10-28 21:00:00+01 | t
 2000-10-29 00:00:00+01 | 2000-10-29 00:00:00+01 | t
 2000-10-29 03:00:00+00 | 2000-10-29 03:00:00+00 | t
 2000-10-29 06:00:00+00 | 2000-10-29 06:00:00+00 | t
 2000-10-29 09:00:00+00 | 2000-10-29 09:00:00+00 | t
 2000-10-29 12:00:00+00 | 2000-10-29 12:00:00+00 | t
 2000-10-29 15:00:00+00 | 2000-10-29 15:00:00+00 | t
 2000-10-29 18:00:00+00 | 2000-10-29 18:00:00+00 | t
 2000-10-29 21:00:00+00 | 2000-10-29 21:00:00+00 | t
 2000-10-30 00:00:00+00 | 2000-10-30 00:00:00+00 | t
 2000-10-30 03:00:00+00 | 2000-10-30 03:00:00+00 | t
 2000-10-30 06:00:00+00 | 2000-10-30 06:00:00+00 | t
 2000-10-30 09:00:00+00 | 2000-10-30 09:00:00+00 | t
 2000-10-30 12:00:00+00 | 2000-10-30 12:00:00+00 | t
 2000-10-30 15:00:00+00 | 2000-10-30 15:00:00+00 | t
 2000-10-30 18:00:00+00 | 2000-10-30 18:00:00+00 | t
 2000-10-30 21:00:00+00 | 2000-10-30 21:00:00+00 | t
 2000-10-31 00:00:00+00 | 2000-10-31 00:00:00+00 | t
(25 rows)

~~About the DST beginning day, the current implementation returns expected result.~~
About the DST beginning day, the current implementation returns wrong results too.

D set timezone = 'Europe/London';
D with t2 as (select t1.ts::timestamptz as ts, time_bucket('10 seconds'::interval, t1.ts::timestamptz, 'Europe/London') as tb from generate_series('2000-03-25 00:00:00'::timestamp, '2000-03-28 00:00:00'::timestamp, '3 hours'::interval) t1 (ts)) select ts, tb, ts = tb as always_should_be_true from t2;
┌──────────────────────────┬──────────────────────────┬───────────────────────┐
│            ts            │            tb            │ always_should_be_true │
│ timestamp with time zone │ timestamp with time zone │        boolean        │
├──────────────────────────┼──────────────────────────┼───────────────────────┤
│ 2000-03-25 00:00:00+00   │ 2000-03-25 00:00:00+00   │ true                  │
│ 2000-03-25 03:00:00+00   │ 2000-03-25 03:00:00+00   │ true                  │
│ 2000-03-25 06:00:00+00   │ 2000-03-25 06:00:00+00   │ true                  │
│ 2000-03-25 09:00:00+00   │ 2000-03-25 09:00:00+00   │ true                  │
│ 2000-03-25 12:00:00+00   │ 2000-03-25 12:00:00+00   │ true                  │
│ 2000-03-25 15:00:00+00   │ 2000-03-25 15:00:00+00   │ true                  │
│ 2000-03-25 18:00:00+00   │ 2000-03-25 18:00:00+00   │ true                  │
│ 2000-03-25 21:00:00+00   │ 2000-03-25 21:00:00+00   │ true                  │
│ 2000-03-26 00:00:00+00   │ 2000-03-26 00:00:00+00   │ true                  │
│ 2000-03-26 03:00:00+01   │ 2000-03-26 02:00:00+01   │ false                 │
│ 2000-03-26 06:00:00+01   │ 2000-03-26 05:00:00+01   │ false                 │
│ 2000-03-26 09:00:00+01   │ 2000-03-26 08:00:00+01   │ false                 │
│ 2000-03-26 12:00:00+01   │ 2000-03-26 11:00:00+01   │ false                 │
│ 2000-03-26 15:00:00+01   │ 2000-03-26 14:00:00+01   │ false                 │
│ 2000-03-26 18:00:00+01   │ 2000-03-26 17:00:00+01   │ false                 │
│ 2000-03-26 21:00:00+01   │ 2000-03-26 20:00:00+01   │ false                 │
│ 2000-03-27 00:00:00+01   │ 2000-03-27 00:00:00+01   │ true                  │
│ 2000-03-27 03:00:00+01   │ 2000-03-27 03:00:00+01   │ true                  │
│ 2000-03-27 06:00:00+01   │ 2000-03-27 06:00:00+01   │ true                  │
│ 2000-03-27 09:00:00+01   │ 2000-03-27 09:00:00+01   │ true                  │
│ 2000-03-27 12:00:00+01   │ 2000-03-27 12:00:00+01   │ true                  │
│ 2000-03-27 15:00:00+01   │ 2000-03-27 15:00:00+01   │ true                  │
│ 2000-03-27 18:00:00+01   │ 2000-03-27 18:00:00+01   │ true                  │
│ 2000-03-27 21:00:00+01   │ 2000-03-27 21:00:00+01   │ true                  │
│ 2000-03-28 00:00:00+01   │ 2000-03-28 00:00:00+01   │ true                  │
├──────────────────────────┴──────────────────────────┴───────────────────────┤
│ 25 rows                                                           3 columns │
└─────────────────────────────────────────────────────────────────────────────┘

hawkfish · 2023-01-03T20:50:19Z

time_bucket result of timestamp on the day DST ended gets greater than the original timestamp.

That's odd - I'm going to pull your code and step into it.

hawkfish · 2023-01-03T21:54:35Z

It looks like the problem is that there is a difference between the number of hours between the TB epoch and the non-DST times on the 29th (7203 == 300d 3h) and the interval that DATE_SUB computes using ICU's date subtraction logic (300h 4h).

hawkfish · 2023-01-03T23:46:41Z

There is a bug in the ICU TSTZ difference code. It is moving the start date forward instead of moving the end date backwards. So when it moves the epoch date forward by 300 days, it gains an hour crossing the spring DST boundary and never gives it back, resulting in an extra hour of difference at the next step. This is not symmetrical with the interval addition code. I'll have a closer look tomorrow and hopefully have a fix.

papparapa · 2023-01-04T01:33:08Z

Thanks for your investigation!
I would like to ask for my future study, did you use lldb or gdb to step into the code?

hawkfish · 2023-01-04T02:36:47Z

Thanks for your investigation!
I would like to ask for my future study, did you use lldb or gdb to step into the code?

Xcode, actually (which wraps lldb)

papparapa · 2023-01-04T10:50:40Z

Xcode, actually (which wraps lldb)

Thanks!

extension/icu/include/icu-datefunc.hpp

extension/icu/icu-timebucket.cpp

hawkfish · 2023-01-04T18:33:17Z

With the bug fix your examples now work, except the last one above, which is wrong (all the dates should be the same).

hawkfish

I think the non-ICU changes are ready to go. Can we split them out into a separate PR?

…me-bucket-function

hawkfish

I think we are down to the µs computation and the DST test conversation.

extension/icu/icu-timebucket.cpp

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test

…me-bucket-function

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test

hawkfish

Just resolve the conversations.

papparapa · 2023-01-20T00:47:05Z

Thanks @hawkfish for the long-term review!

Mytherin · 2023-01-21T12:25:42Z

Thanks!

Swoorup · 2023-01-21T13:01:06Z

Woah 😍

Mytherin requested a review from hawkfish December 12, 2022 12:40

hawkfish reviewed Dec 12, 2022

View reviewed changes

papparapa requested a review from hawkfish December 18, 2022 23:45

hawkfish reviewed Dec 19, 2022

View reviewed changes

extension/icu/icu-timebucket.cpp Outdated Show resolved Hide resolved

extension/icu/icu-timebucket.cpp Outdated Show resolved Hide resolved

src/function/scalar/date/time_bucket.cpp Outdated Show resolved Hide resolved

hawkfish reviewed Dec 23, 2022

View reviewed changes

papparapa commented Dec 24, 2022

View reviewed changes

src/function/scalar/date/time_bucket.cpp Outdated Show resolved Hide resolved

hawkfish added the feature label Dec 29, 2022

hawkfish reviewed Dec 29, 2022

View reviewed changes

extension/icu/include/icu-datefunc.hpp Outdated Show resolved Hide resolved

extension/icu/icu-timezone.cpp Outdated Show resolved Hide resolved

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test Show resolved Hide resolved

hawkfish reviewed Jan 4, 2023

View reviewed changes

extension/icu/include/icu-datefunc.hpp Outdated Show resolved Hide resolved

extension/icu/icu-timebucket.cpp Show resolved Hide resolved

hawkfish reviewed Jan 5, 2023

View reviewed changes

papparapa mentioned this pull request Jan 6, 2023

Add non ICU time_bucket function #5835

Merged

papparapa added 5 commits January 6, 2023 10:16

Restore ICU time_bucket

d39c166

Delete non-ICU time_bucket

516c5d2

Fix to use SubtractOperatorOverflowCheck

2a24b6a

Add exception

c317109

Remove unused code

468c552

papparapa added 4 commits January 7, 2023 02:33

Fix incorrect comment

eaa680f

Add DUCKDB_API specifier to SubtractOperatorOverflowCheck

91336a5

Merge branch 'master' of https://github.com/duckdb/duckdb into add-ti…

5ba120a

…me-bucket-function

Restore non-ICU time_bucket

4b9c00d

hawkfish reviewed Jan 9, 2023

View reviewed changes

extension/icu/icu-timebucket.cpp Show resolved Hide resolved

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test Show resolved Hide resolved

hawkfish mentioned this pull request Jan 11, 2023

ICU TSTZ + INTERVAL From Mars #5887

Closed

papparapa added 4 commits January 12, 2023 13:42

Merge branch 'master' of https://github.com/duckdb/duckdb into add-ti…

78d8340

…me-bucket-function

Fix to use epoch microseconds subtraction

845ad1e

Merge branch 'master' of https://github.com/duckdb/duckdb into add-ti…

2e7e1f0

…me-bucket-function

Add ICU time_bucket tests

f813c54

papparapa requested a review from hawkfish January 16, 2023 14:56

papparapa commented Jan 16, 2023

View reviewed changes

test/sql/function/timestamp/test_icu_time_bucket_timestamptz.test Show resolved Hide resolved

papparapa added 2 commits January 17, 2023 00:09

Empty commit to trigger CI

eca104b

Remove unnecessary DUCKDB_API specifier

75a0f6f

hawkfish approved these changes Jan 19, 2023

View reviewed changes

Mytherin merged commit c567e87 into duckdb:master Jan 21, 2023

papparapa deleted the add-time-bucket-function branch January 22, 2023 04:26

papparapa mentioned this pull request Jan 22, 2023

Add time_bucket function docs duckdb/duckdb-web#503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add time bucket function #5665

Add time bucket function #5665

papparapa commented Dec 12, 2022 •

edited

hawkfish left a comment

papparapa commented Dec 12, 2022

papparapa commented Dec 18, 2022

hawkfish left a comment

hawkfish commented Dec 19, 2022

hawkfish left a comment

papparapa commented Dec 20, 2022

hawkfish commented Dec 23, 2022

hawkfish left a comment

papparapa commented Jan 3, 2023 •

edited

hawkfish commented Jan 3, 2023

hawkfish commented Jan 3, 2023

hawkfish commented Jan 3, 2023 •

edited

papparapa commented Jan 4, 2023 •

edited

hawkfish commented Jan 4, 2023

papparapa commented Jan 4, 2023

hawkfish commented Jan 4, 2023

hawkfish left a comment

hawkfish left a comment

hawkfish left a comment

papparapa commented Jan 20, 2023

Mytherin commented Jan 21, 2023

Swoorup commented Jan 21, 2023

Add time bucket function #5665

Add time bucket function #5665

Conversation

papparapa commented Dec 12, 2022 • edited

hawkfish left a comment

Choose a reason for hiding this comment

papparapa commented Dec 12, 2022

papparapa commented Dec 18, 2022

hawkfish left a comment

Choose a reason for hiding this comment

hawkfish commented Dec 19, 2022

hawkfish left a comment

Choose a reason for hiding this comment

papparapa commented Dec 20, 2022

hawkfish commented Dec 23, 2022

hawkfish left a comment

Choose a reason for hiding this comment

papparapa commented Jan 3, 2023 • edited

hawkfish commented Jan 3, 2023

hawkfish commented Jan 3, 2023

hawkfish commented Jan 3, 2023 • edited

papparapa commented Jan 4, 2023 • edited

hawkfish commented Jan 4, 2023

papparapa commented Jan 4, 2023

hawkfish commented Jan 4, 2023

hawkfish left a comment

Choose a reason for hiding this comment

hawkfish left a comment

Choose a reason for hiding this comment

hawkfish left a comment

Choose a reason for hiding this comment

papparapa commented Jan 20, 2023

Mytherin commented Jan 21, 2023

Swoorup commented Jan 21, 2023

papparapa commented Dec 12, 2022 •

edited

papparapa commented Jan 3, 2023 •

edited

hawkfish commented Jan 3, 2023 •

edited

papparapa commented Jan 4, 2023 •

edited