[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
Attachments:
0001-Replace-float8-with-int-in-date2isoweek-and-date2iso.patchtext/x-patchDownload
From e3aac9f2ca12b60ff35d01ea69e4ffbaeea30656 Mon Sep 17 00:00:00 2001
From: Sergey Fukanchik <s.fukanchik@postgrespro.ru>
Date: Sat, 12 Jul 2025 13:25:18 +0300
Subject: [PATCH] Replace float8 with int in date2isoweek and date2isoyear
---
src/backend/utils/adt/timestamp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 0a5848a4ab2..6194769b021 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5312,7 +5312,7 @@ isoweekdate2date(int isoweek, int wday, int *year, int *mon, int *mday)
int
date2isoweek(int year, int mon, int mday)
{
- float8 result;
+ int result;
int day0,
day4,
dayn;
@@ -5355,7 +5355,7 @@ date2isoweek(int year, int mon, int mday)
result = (dayn - (day4 - day0)) / 7 + 1;
}
- return (int) result;
+ return result;
}
@@ -5367,7 +5367,7 @@ date2isoweek(int year, int mon, int mday)
int
date2isoyear(int year, int mon, int mday)
{
- float8 result;
+ int result;
int day0,
day4,
dayn;
--
2.34.1
=?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
Show quoted text
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
Attachments:
v2-0001-Replace-float8-with-int-in-date2isoweek-and-date2.patchtext/x-patch; charset=UTF-8; name=v2-0001-Replace-float8-with-int-in-date2isoweek-and-date2.patchDownload
From 501da79f2a794b66cb4ebc3673a60859070a59df Mon Sep 17 00:00:00 2001
From: Sergey Fukanchik <s.fukanchik@postgrespro.ru>
Date: Sun, 13 Jul 2025 16:52:09 +0300
Subject: [PATCH v2] Replace float8 with int in date2isoweek and date2isoyear
---
src/backend/utils/adt/timestamp.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 0a5848a4ab2..7420891092b 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5312,7 +5312,7 @@ isoweekdate2date(int isoweek, int wday, int *year, int *mon, int *mday)
int
date2isoweek(int year, int mon, int mday)
{
- float8 result;
+ int week;
int day0,
day4,
dayn;
@@ -5338,13 +5338,13 @@ date2isoweek(int year, int mon, int mday)
day0 = j2day(day4 - 1);
}
- result = (dayn - (day4 - day0)) / 7 + 1;
+ week = (dayn - (day4 - day0)) / 7 + 1;
/*
* Sometimes the last few days in a year will fall into the first week of
* the next year, so check for this.
*/
- if (result >= 52)
+ if (week >= 52)
{
day4 = date2j(year + 1, 1, 4);
@@ -5352,10 +5352,10 @@ date2isoweek(int year, int mon, int mday)
day0 = j2day(day4 - 1);
if (dayn >= day4 - day0)
- result = (dayn - (day4 - day0)) / 7 + 1;
+ week = (dayn - (day4 - day0)) / 7 + 1;
}
- return (int) result;
+ return week;
}
@@ -5367,7 +5367,7 @@ date2isoweek(int year, int mon, int mday)
int
date2isoyear(int year, int mon, int mday)
{
- float8 result;
+ int week;
int day0,
day4,
dayn;
@@ -5395,13 +5395,13 @@ date2isoyear(int year, int mon, int mday)
year--;
}
- result = (dayn - (day4 - day0)) / 7 + 1;
+ week = (dayn - (day4 - day0)) / 7 + 1;
/*
* Sometimes the last few days in a year will fall into the first week of
* the next year, so check for this.
*/
- if (result >= 52)
+ if (week >= 52)
{
day4 = date2j(year + 1, 1, 4);
--
2.34.1
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
Show quoted text
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