Bruce: Thanks for tackling this issue. The patch looks good to me. When you have time, can you include the places which were not covered by the current diff ?
Cheers On Fri, Apr 2, 2021 at 11:06 AM Bruce Momjian <br...@momjian.us> wrote: > On Thu, Apr 1, 2021 at 09:46:58PM -0700, Bryn Llewellyn wrote: > > Or am I misunderstanding something? > > > > Try this. The result of each “select” is shown as the trailing comment > on the > > same line. I added whitespace by hand to line up the fields. > > > > select interval '-1.7 years'; -- -1 years -8 > mons > > > > select interval '29.4 months'; -- 2 years 5 > mons 12 > > days > > > > select interval '-1.7 years 29.4 months'; -- 8 > mons 12 > > days << wrong > > select interval '29.4 months -1.7 years'; -- 9 > mons 12 > > days > > > > select interval '-1.7 years' + interval '29.4 months'; -- 9 > mons 12 > > days > > select interval '29.4 months' + interval '-1.7 years'; -- 9 > mons 12 > > days > > > > As I reason it, the last four “select” statements are all semantically > the > > same. They’re just different syntaxes to add the two intervals the the > first > > two “select” statements use separately. There’s one odd man out. And I > reason > > this one to be wrong. Is there a flaw in my reasoning? > > [Thread moved to hackers.] > > Looking at your report, I thought I could easily find the cause, but it > wasn't obvious. What is happening is that when you cast a float to an > integer in C, it rounds toward zero, meaning that 8.4 rounds to 8 and > -8.4 rounds to -8. The big problem here is that -8.4 is rounding in a > positive direction, while 8.4 rounds in a negative direction. See this: > > int(int(-8.4) + 29) > 21 > int(int(29) + -8.4) > 20 > > When you do '-1.7 years' first, it become -8, and then adding 29 yields > 21. In the other order, it is 29 - 8.4, which yields 20.6, which > becomes 20. I honestly had never studied this interaction, though you > would think I would have seen it before. One interesting issue is that > it only happens when the truncations happen to values with different > signs --- if they are both positive or negative, it is fine. > > The best fix I think is to use rint()/round to round to the closest > integer, not toward zero. The attached patch does this in a few places, > but the code needs more research if we are happy with this approach, > since there are probably other cases. Using rint() does help to produce > more accurate results, thought the regression tests show no change from > this patch. > > > Further… there’s a notable asymmetry. The fractional part of “1.7 years” > is 8.4 > > months. But the fractional part of the months value doesn’t spread > further down > > into days. However, the fractional part of “29.4 months” (12 days) _does_ > > spread further down into days. What’s the rationale for this asymmetry? > > Yes, looking at the code, it seems we only spill down to one unit, not > more. I think we need to have a discussion if we want to change that. > I think the idea was that if you specify a non-whole number, you > probably want to spill down one level, but don't want it spilling all > the way to milliseconds, which is certainly possible. > > > I can’t see that my observations here can be explained by the difference > > between calendar time and clock time. Here I’m just working with > non-metric > > units like feet and inches. One year is just defined as 12 months. And > one > > month is just defined as 30 days. All that stuff about adding a month to > > 3-Feb-2020 taking you to 3-Mar-2020 (same for leap years an non-leap > years) , > > and that other stuff about adding one day to 23:00 on the day before the > > “spring forward” moment taking you to 23:00 on the next day (i.w. when > > intervals are added to timestamps) is downstream of simply adding two > > intervals. > > Ah, seems you have done some research. ;-) > > -- > Bruce Momjian <br...@momjian.us> https://momjian.us > EDB https://enterprisedb.com > > If only the physical world exists, free will is an illusion. > >
