Align gapfill bucket generation with time_bucket

[antekresic]

In certain scenarios, when generating buckets with monthly buckets and different timezones, gapfill
would create timestamps which don't align with time_bucket and thus potentially generating multiple
rows for an individual month. Instead of relying on previous timestamp to generate the next one, now
we generate them always from the start point which will make us align with time_bucket buckets.

[github-actions]

@mkindahl, @gayyappan: please review this pull request.

Powered by pull-review

[codecov]

Codecov Report

Merging #6155 (ef75c52) into main (a409065) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #6155      +/-   ##
==========================================
- Coverage   65.05%   65.03%   -0.03%     
==========================================
  Files         246      246              
  Lines       56949    56955       +6     
  Branches    12620    12621       +1     
==========================================
- Hits        37048    37040       -8     
- Misses      18030    18066      +36     
+ Partials     1871     1849      -22     

Files	Coverage Δ
tsl/src/nodes/gapfill/gapfill_exec.c	94.68% <100.00%> (+0.05%)	⬆️

... and 60 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[svenklemm]

Hmm the interval handling seems a bit convoluted, can't we initialize it with gapfill_interval and then add to it, no need to ever memset or palloc

[mkindahl]

I wonder if the computation is correct. Doing a naïve version of the computation gives me a completely different result.

[mkindahl]

I am slightly unsure what was causing the previous issue, but it looks like this approach suffer from the same problem as the previous one, or I might be misunderstanding what difference it makes. Was the original problem due to a precision issue, or something else?

Don't you want to do a computation along the lines gapfill_start + n * gapfill_interval?

[antekresic]

Previous implementation depended on previous timestamp to calculate the next one. This becomes a problem in case of variable bucket sizes like months when the bucket lands on the last day of the month (which is a different day every time).

The computation you suggest is the same one I'm doing here but instead of maintaining the position (n), I maintain the offset by incrementing it for every generated bucket.

My implementation seemed simpler in terms of calculating the next timestamp so that's why I went with it.

[mkindahl]

I see that this is the last day of the month rather than the 29:th, but is correct? When I do a test I get the following result:

mats=# select day::date, day::date + '1 month'::interval, day::date + '2 month'::interval from (values ('2023-02-28'), ('2023-08-31'), ('2023-01-29')) as d(day);
    day     |      ?column?       |      ?column?       
------------+---------------------+---------------------
 2023-02-28 | 2023-03-28 00:00:00 | 2023-04-28 00:00:00
 2023-08-31 | 2023-09-30 00:00:00 | 2023-10-31 00:00:00
 2023-01-29 | 2023-02-28 00:00:00 | 2023-03-29 00:00:00
(3 rows)

[antekresic]

I don't understand why is it not correct? If these are monthly buckets, should they not start/end on the last day of the month for every bucket?

Even your example shows that here:
2023-08-31 | 2023-09-30 00:00:00 | 2023-10-31 00:00:00

[mkindahl]

Sorry, the case above is actually correct so it is not a good illustration of the problem I was wondering about.

I implemented this in PL/pgSQL to convince myself this works the way it should and it seems like that.

CREATE OR REPLACE FUNCTION advance_offset(start date, steps int, ival interval)
RETURNS date
LANGUAGE plpgsql AS $$
DECLARE
    off interval;
    count int := steps - 1;
BEGIN
    off := ival;
    LOOP
        off := off + ival;
	count := count - 1;
	IF count = 0 THEN
	    EXIT;
	END IF;
    END LOOP;
    RETURN start + off;
END
$$;

CREATE OR REPLACE FUNCTION advance_date(start date, steps int, ival interval)
RETURNS date
LANGUAGE plpgsql AS $$
DECLARE
    ts date;
    count int := steps;
BEGIN
    ts := start;
    LOOP
        ts := ts + ival;
	count := count - 1;
	IF count = 0 THEN
	    EXIT;
	END IF;
    END LOOP;
    RETURN ts;
END
$$;

Which then gives the following result:

mats=# SELECT start,
       advance_date(start::date, 10, '1 month'::interval),
       advance_offset(start::date, 10, '1 month'::interval),
       CAST(start::date + 10 * '1 month'::interval AS date)
  FROM (VALUES ('2023-01-28'), ('2023-01-29'), ('2023-01-30'), ('2023-01-31')) AS tbl(start);
   start    | advance_date | advance_offset |    date    
------------+--------------+----------------+------------
 2023-01-28 | 2023-11-28   | 2023-11-28     | 2023-11-28
 2023-01-29 | 2023-11-28   | 2023-11-29     | 2023-11-29
 2023-01-30 | 2023-11-28   | 2023-11-30     | 2023-11-30
 2023-01-31 | 2023-11-28   | 2023-11-30     | 2023-11-30
(4 rows)

The middle one is the original computation, which illustrates the problem, and the last one is the new implementation. I will approve the patch, but it would be good if you could use some extra dates to the test similar to the ones I have here so that we make sure that it works both when the date lands on the "last day of the month" as part of the computation and when the starting date is not the last day of the month (and the end result should not end on the last day of the month).

[mkindahl]

Sorry, the case above is actually correct so it is not a good illustration of the problem I was wondering about.

I implemented this in PL/pgSQL to convince myself this works the way it should and it seems like that.

CREATE OR REPLACE FUNCTION advance_offset(start date, steps int, ival interval)
RETURNS date
LANGUAGE plpgsql AS $$
DECLARE
    off interval;
    count int := steps - 1;
BEGIN
    off := ival;
    LOOP
        off := off + ival;
	count := count - 1;
	IF count = 0 THEN
	    EXIT;
	END IF;
    END LOOP;
    RETURN start + off;
END
$$;

CREATE OR REPLACE FUNCTION advance_date(start date, steps int, ival interval)
RETURNS date
LANGUAGE plpgsql AS $$
DECLARE
    ts date;
    count int := steps;
BEGIN
    ts := start;
    LOOP
        ts := ts + ival;
	count := count - 1;
	IF count = 0 THEN
	    EXIT;
	END IF;
    END LOOP;
    RETURN ts;
END
$$;

Which then gives the following result:

mats=# SELECT start,
       advance_date(start::date, 10, '1 month'::interval),
       advance_offset(start::date, 10, '1 month'::interval),
       CAST(start::date + 10 * '1 month'::interval AS date)
  FROM (VALUES ('2023-01-28'), ('2023-01-29'), ('2023-01-30'), ('2023-01-31')) AS tbl(start);
   start    | advance_date | advance_offset |    date    
------------+--------------+----------------+------------
 2023-01-28 | 2023-11-28   | 2023-11-28     | 2023-11-28
 2023-01-29 | 2023-11-28   | 2023-11-29     | 2023-11-29
 2023-01-30 | 2023-11-28   | 2023-11-30     | 2023-11-30
 2023-01-31 | 2023-11-28   | 2023-11-30     | 2023-11-30
(4 rows)

The middle one is the original computation, which illustrates the problem, and the last one is the new implementation. I will approve the patch, but it would be good if you could use some extra dates to the test similar to the ones I have here so that we make sure that it works both when the date lands on the "last day of the month" as part of the computation and when the starting date is not the last day of the month (and the end result should not end on the last day of the month).

[timescale-automation]

Automated backport to 2.12.x not done: cherry-pick failed.

Git status

HEAD detached at origin/2.12.x
You are currently cherry-picking commit 65ecda93e.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   .unreleased/PR_6155
	modified:   tsl/src/nodes/gapfill/gapfill_exec.c
	modified:   tsl/src/nodes/gapfill/gapfill_internal.h
	modified:   tsl/test/shared/expected/gapfill.out
	modified:   tsl/test/shared/sql/gapfill.sql

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   tsl/test/shared/expected/gapfill-14.out
	deleted by us:   tsl/test/shared/expected/gapfill-15.out
	deleted by us:   tsl/test/shared/expected/gapfill-16.out

Job log