Обсуждение: [PATCH] replace float8 with int in date2isoweek() and date2isoyear()

Hi PG hackers,

I found suspicious use of float8 in date2isoweek() and date2isoyear(). In both
cases float8 is only used for storing the value, while the entire calculation
on the right happens in integers:

 float8 result = (dayn - (day4 - day0)) / 7 + 1;

At the end date2isoweek() returns `result' converted back to int:

 return (int) result;

float8 here is confusing and a bit slow.

I run a benchmark with optimization -O3, and found that using int is 3% faster:

BM_Float/1000/3/8_mean         5.12 ns         5.12 ns          100
BM_Int/1000/3/8_mean            4.95 ns         4.95 ns          100
BM_Long/1000/3/8_mean         4.96 ns         4.96 ns          100

Without optimization (-O0), speedup even better around 30%:

BM_Float/1000/3/8_mean         19.6 ns         19.6 ns          100
BM_Int/1000/3/8_mean             14.8 ns         14.8 ns          100
BM_Long/1000/3/8_mean          16.7 ns         16.7 ns          100

Additionally, as a paranoia measure I run a comparison test with the following
ranges: year={-100000,100000}, month={-100,+100}, day={-100,+100}.
As expected float and int did not produce any difference.

Attached patch replaces `float8 result' with `int result'.

I think there is no need in adding an extra test case here, because
date2isoweek and date2isoyear are covered by three regression tests:
* date
 SELECT f1 as "date",
     ...
     date_part('isoyear', f1) AS isoyear,
     date_part('week', f1) AS week,
     date_part('dow', f1) AS dow,
     date_part('isodow', f1) AS isodow,
 ...
     FROM date_tbl;
* timestamp
 SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc;
* timestamptz
 SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS week_trunc;
---
Sergey

=?utf-8?q?=D0=A4=D1=83=D0=BA=D0=B0=D0=BD=D1=87=D0=B8=D0=BA_=D0=A1=D0=B5=D1=80=D0=B3=D0=B5=D0=B9?=
<s.fukanchik@postgrespro.ru>writes: 
> Hi PG hackers,
> I found suspicious use of float8 in date2isoweek() and date2isoyear(). In both
> cases float8 is only used for storing the value, while the entire calculation
> on the right happens in integers:

>  float8 result = (dayn - (day4 - day0)) / 7 + 1;

> At the end date2isoweek() returns `result' converted back to int:

>  return (int) result;

> float8 here is confusing and a bit slow.

I looked into our git history to try to find out why it's like this.
The answer seems to be that commit dffd8cac3 created date2isoweek()
by splitting out pre-existing code that had been in timestamp_part().
In that context the code had been using a float8 "result" variable
that was shared with other switch cases, and that variable's type
was just blindly copied into date2isoweek().  Then 1c757c49f again
copied-and-pasted while creating date2isoyear().

I agree with getting rid of the unnecessary usage of float8 here,
but there's another aspect that's bugging me: "result" is a totally
misleading variable name in date2isoyear(), because it's *not*
the function's result.  I'm inclined to rename it to "week", and
then to keep these functions looking as parallel as possible,
I'd probably do the same in date2isoweek().

> I think there is no need in adding an extra test case here, because
> date2isoweek and date2isoyear are covered by three regression tests:

Agreed, the code coverage report shows these are covered.

            regards, tom lane

Sure,

I'm attaching v2 of the patch with "result" renamed to "week".

--

Sergey

On 7/12/25 18:15, Tom Lane wrote:
> =?utf-8?q?=D0=A4=D1=83=D0=BA=D0=B0=D0=BD=D1=87=D0=B8=D0=BA_=D0=A1=D0=B5=D1=80=D0=B3=D0=B5=D0=B9?=
<s.fukanchik@postgrespro.ru>writes:
 
>> Hi PG hackers,
>> I found suspicious use of float8 in date2isoweek() and date2isoyear(). In both
>> cases float8 is only used for storing the value, while the entire calculation
>> on the right happens in integers:
>>   float8 result = (dayn - (day4 - day0)) / 7 + 1;
>> At the end date2isoweek() returns `result' converted back to int:
>>   return (int) result;
>> float8 here is confusing and a bit slow.
> I looked into our git history to try to find out why it's like this.
> The answer seems to be that commit dffd8cac3 created date2isoweek()
> by splitting out pre-existing code that had been in timestamp_part().
> In that context the code had been using a float8 "result" variable
> that was shared with other switch cases, and that variable's type
> was just blindly copied into date2isoweek().  Then 1c757c49f again
> copied-and-pasted while creating date2isoyear().
>
> I agree with getting rid of the unnecessary usage of float8 here,
> but there's another aspect that's bugging me: "result" is a totally
> misleading variable name in date2isoyear(), because it's *not*
> the function's result.  I'm inclined to rename it to "week", and
> then to keep these functions looking as parallel as possible,
> I'd probably do the same in date2isoweek().
>
>> I think there is no need in adding an extra test case here, because
>> date2isoweek and date2isoyear are covered by three regression tests:
> Agreed, the code coverage report shows these are covered.
>
>             regards, tom lane

Sergey Fukanchik <s.fukanchik@postgrespro.ru> writes:
> I'm attaching v2 of the patch with "result" renamed to "week".

I pushed it already, actually.

            regards, tom lane

Thank you!

--

Sergey

On 7/13/25 18:39, Tom Lane wrote:
> Sergey Fukanchik <s.fukanchik@postgrespro.ru> writes:
>> I'm attaching v2 of the patch with "result" renamed to "week".
> I pushed it already, actually.
>
>             regards, tom lane