Re: Fix for edge case in date_bin() function

2024-02-29 Thread Tom Lane
Moaaz Assali writes: > However, I don't see the issue with the INT64 -> UINT64 mapping. The > current implementation results in integer overflows (errors instead after > the recent patch) even for valid timestamps where the result of date_bin() > is also another valid timestamp. > On the other

Re: Fix for edge case in date_bin() function

2024-02-28 Thread Moaaz Assali
Hello Tom, Thanks for the quick patch! You're right. The stride_usecs calculation, tm_delta += ustride_usecs, and final result calculations can overflow and need a guard. However, I don't see the issue with the INT64 -> UINT64 mapping. The current implementation results in integer overflows

Re: Fix for edge case in date_bin() function

2024-02-28 Thread Tom Lane
Moaaz Assali writes: > - I have used INT64 -> UINT64 mapping in order to ensure no integer > overflows are possible. I don't think I trust this idea, and in any case it doesn't remove all the overflow hazards: the reduction of the stride interval to microseconds can overflow, and the final

Re: Fix for edge case in date_bin() function

2024-02-28 Thread Moaaz Assali
Yeah you are right about the integer overflow. Consider this query: select date_bin('15 minutes'::interval, timestamp '294276-12-30 10:24:00', timestamp '4000-12-20 23:00:00 BC'); It will return 294276-12-30 10:31:49.551616 when it should be 294276-12-30 10:15:00, which happens because source

Re: Fix for edge case in date_bin() function

2024-02-27 Thread Tom Lane
I wrote: > Hmm, yeah. The "stride_usecs > 1" test seems like it's a partial > attempt to account for this that is probably redundant given the > additional condition. Also, can we avoid computing tm_diff % > stride_usecs twice? Maybe the compiler is smart enough to remove the > common

Re: Fix for edge case in date_bin() function

2024-02-27 Thread Tom Lane
Moaaz Assali writes: > The date_bin() function has a bug where it returns an incorrect binned date > when both of the following are true: > 1) the origin timestamp is before the source timestamp > 2) the origin timestamp is exactly equivalent to some valid binned date in > the set of binned dates

Re: Fix for edge case in date_bin() function

2024-02-27 Thread Moaaz Assali
Hello Daniel, I have added a test case for this in timestamp.sql and timestamp.out, and tests pass when using the bug fix patch in the first email. I have attached a new patch in this email below with the new tests only (doesn't include the bug fix). P.S. I forgot to CC the mailing list in my

Re: Fix for edge case in date_bin() function

2024-02-27 Thread Daniel Gustafsson
> On 27 Feb 2024, at 09:42, Moaaz Assali wrote: > To account for this edge, we simply add another condition in the if statement > to not perform the subtraction by one stride interval if the time difference > is divisible by the stride. I only skimmed the patch, but I recommend adding a

Fix for edge case in date_bin() function

2024-02-27 Thread Moaaz Assali
Hello, The date_bin() function has a bug where it returns an incorrect binned date when both of the following are true: 1) the origin timestamp is before the source timestamp 2) the origin timestamp is exactly equivalent to some valid binned date in the set of binned dates that date_bin() can