Introduce experimental time_bucket_ng() function

[afiskon]

Introduce experimental time_bucket_ng() function

This patch adds time_bucket_ng() function to the experimental
schema. The "ng" part stands for "next generation". Unlike
current time_bucket() implementation the _ng version will support
months, years and timezones.

Current implementation doesn't claim to be complete. For instance,
it doesn't support timezones yet. The reasons to commit it in it's
current state are 1) to shorten the feedback loop 2) to start
experimenting with monthly buckets are soon as possible,
3) to reduce the unnecessary work of rebasing and resolving
conflicts 4) to make the work easier to the reviewers

See also #3211 (comment)

[codecov]

Codecov Report

Merging #3211 (35bc689) into master (cf5626b) will increase coverage by 0.00%.
The diff coverage is 96.15%.

❗ Current head 35bc689 differs from pull request most recent head 95058a7. Consider uploading reports for the commit 95058a7 to get more accurate results

@@           Coverage Diff           @@
##           master    #3211   +/-   ##
=======================================
  Coverage   90.61%   90.61%           
=======================================
  Files         211      212    +1     
  Lines       35536    35588   +52     
=======================================
+ Hits        32200    32249   +49     
- Misses       3336     3339    +3     

Impacted Files	Coverage Δ
src/time_bucket_ng.c	96.15% <96.15%> (ø)
tsl/src/nodes/data_node_dispatch.c	97.08% <0.00%> (-0.25%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71e8f13...95058a7. Read the comment docs.

[binakot]

@afiskon, Hello!
Will this function be in addition to time_bucket()?
What are the differences from it?

[afiskon]

@binakot Many users requested time_bucket but with the support of variable-sized buckets (e.g. months) or timezones and we try our best to provide this functionality. At this point, ts_date_trunc() is just an extended version of date_trunc(). You can do something like ts_date_trunc('1 year 3 months', ...) which can't be done with date_trunc(). The main difference from time_bucket() is that time_bucket() always works with fixed-sized buckets (hours, days, ...) and thus can be used in continuous aggregates. This PR is a kind of work in progress. We experiment with the code, gather feedback, and see what can be done next.

[NunoFilipeSantos]

This will be a candidate to our experimental schema, correct @mkindahl ?
If so, @afiskon please move the PR back to in progress.

[mkindahl]

This will be a candidate to our experimental schema, correct @mkindahl ?
If so, @afiskon please move the PR back to in progress.

I think this is suitable for the experimental schema, yes.

[mfundul]

I don't quite like the heavy use of DirectFunctionCallX() calls. In the original time_bucket.c code the kernel of the computation is with preprocessor macros (e.g. #define TIME_BUCKET(period, timestamp, offset, min, max, result) \).

I think we could achieve the same overhead as the macros by using static inline functions in the place of those FMGR calls.

[afiskon]

If this is a replacement for time_bucket, why do we only support 1 day as the minimum bucket size? I see in the code that timestamps are always converted to a Date type internally, which gives the granularity of days and drops timezone. Is this really what we want to do here?

What if someone wants to bucket 24 hours, 10 hours, 3 minutes, etc?

I see in the commit message that timezone support will be added later, but what about smaller buckets than 1 day?

@erimatnor Many thanks for the review! This is a good question. I think I answered this on a standup, but apparently on the one where not the entire team was present. My bad.

This is not the final version of the implementation. My current goal is to make the following query work properly:

CREATE MATERIALIZED VIEW conditions_summary_monthly
WITH (timescaledb.continuous) AS
SELECT device,
       timescaledb_experimental.time_bucket_ng('1 month', tstamp) AS bucket,
       AVG(temperature),
       MAX(temperature),
       MIN(temperature)
FROM conditions
GROUP BY device, bucket;

Then I will cover it with tests and submit another PR. Now we have experimental support of 1-month buckets.

The next step is to make N-month buckets work. Same idea - separate tests, separate PR, separate review, and we have experimental support of N-month buckets.

Then we can work on years - '1 year' buckets first, then '1 year, 3 months' and more complicated scenarios. Separate tests, separate code reviews, etc. Then we can start working for instance on timezones.

Days and hours can be added at any time since we already know they work. We just don't need to support (+ test, review) them in time_bucket_ng at the moment.

So the general idea is to decompose the large task, add the feature in incremental steps. Starting from the parts where there are the most unknowns, and thus where the risks are higher.

[afiskon]

I don't quite like the heavy use of DirectFunctionCallX() calls. In the original time_bucket.c code the kernel of the computation is with preprocessor macros (e.g. #define TIME_BUCKET(period, timestamp, offset, min, max, result) \).

I think we could achieve the same overhead as the macros by using static inline functions in the place of those FMGR calls.

@mfundul Many thanks for the review. @erimatnor didn't like these calls as well but for different reasons. As I explained above #3211 (comment) we will not keep this code forever. There will be no DirectFunctionCallX in the final implementation. Please see the full reply.

[erimatnor]

If this is a replacement for time_bucket, why do we only support 1 day as the minimum bucket size? I see in the code that timestamps are always converted to a Date type internally, which gives the granularity of days and drops timezone. Is this really what we want to do here?
What if someone wants to bucket 24 hours, 10 hours, 3 minutes, etc?
I see in the commit message that timezone support will be added later, but what about smaller buckets than 1 day?

@erimatnor Many thanks for the review! This is a good question. I think I answered this on a standup, but apparently on the one where not the entire team was present. My bad.

This is not the final version of the implementation. My current goal is to make the following query work properly:

CREATE MATERIALIZED VIEW conditions_summary_monthly
WITH (timescaledb.continuous) AS
SELECT device,
       timescaledb_experimental.time_bucket_ng('1 month', tstamp) AS bucket,
       AVG(temperature),
       MAX(temperature),
       MIN(temperature)
FROM conditions
GROUP BY device, bucket;

Then I will cover it with tests and submit another PR. Now we have experimental support of 1-month buckets.

I am not following the connection to continuous aggregates here. If we are removing limitations in the cagg implementation, we could just use, e.g., date_trunc instead, which already support weeks, months, years, and time zones, and is already readily available and familiar to users. I think support for date_trunc would provide immediate value over an experimental bucketing function. It would be helpful to provide this context in the PR description because it is easy to miss information if one has to dig through prior discussions to find it.

[afiskon]

I am not following the connection to continuous aggregates here. If we are removing limitations in the cagg implementation, we could just use, e.g., date_trunc instead, ...

@erimatnor Unfortunately, we can't use date_trunc in the general case because it doesn't support things like '5 months' and/or origin argument. Thus it's not very helpful for purposes named above. Although I think supporting date_trunc as well as our function at some point would be neat indeed.