Hi Joseph, thanks for addressing comments. On Sat, Apr 1, 2023 at 10:53 PM Joseph Koshakow <kosh...@gmail.com> wrote: > So I added a check for FLOAT8_FITS_IN_INT64() and a test with this > scenario.
I like that. Thanks. > > For what it's worth I think that 2147483647 months only became a valid > interval in v15 as part of this commit [0]. It's also outside of the > documented valid range [1], which is > [-178000000 years, 178000000 years] or > [-14833333 months, 14833333 months]. you mean +/-2136000000 months :). In that sense the current code actually fixes a bug introduced in v15. So I am fine with it. > > The rationale for only checking the month's field is that it's faster > than checking all three fields, though I'm not entirely sure if it's > the right trade-off. Any thoughts on this? Hmm, comparing one integer is certainly faster than comparing three. We do that check at least once per interval operation. So the thrice CPU cycles might show some impact when millions of rows are processed. Given that we have clear documentation of bounds, just using months field is fine. If needed we can always expand it later. > > > There's some way to avoid separate checks for infinite-ness of > > interval and factor and use a single block using some integer > > arithmetic. But I think this is more readable. So I avoided doing > > that. Let me know if this works for you. > > I think the patch looks good, I've combined it with the existing patch. > > > Also added some test cases. > > I didn't see any tests in the patch, did you forget to include it? Sorry I forgot to include those. Attached. Please see my reply to your latest email as well. -- Best Wishes, Ashutosh Bapat
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out index 0adfe5f9fb..ebb4208ad7 100644 --- a/src/test/regress/expected/interval.out +++ b/src/test/regress/expected/interval.out @@ -2161,6 +2161,26 @@ SELECT interval '-infinity' * 'nan'; ERROR: interval out of range SELECT interval '-1073741824 months -1073741824 days -4611686018427387904 us' * 2; ERROR: interval out of range +SELECT interval 'infinity' * 0; +ERROR: interval out of range +SELECT interval '-infinity' * 0; +ERROR: interval out of range +SELECT interval '0 days' * 'infinity'::float; +ERROR: interval out of range +SELECT interval '0 days' * '-infinity'::float; +ERROR: interval out of range +SELECT interval '5 days' * 'infinity'::float; + ?column? +---------- + infinity +(1 row) + +SELECT interval '5 days' * '-infinity'::float; + ?column? +----------- + -infinity +(1 row) + SELECT interval 'infinity' / 'infinity'; ERROR: interval out of range SELECT interval 'infinity' / '-infinity'; diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql index 1e1d8560bf..549ceb57c1 100644 --- a/src/test/regress/sql/interval.sql +++ b/src/test/regress/sql/interval.sql @@ -690,6 +690,12 @@ SELECT -interval '-2147483647 months -2147483647 days -9223372036854775807 us'; SELECT interval 'infinity' * 'nan'; SELECT interval '-infinity' * 'nan'; SELECT interval '-1073741824 months -1073741824 days -4611686018427387904 us' * 2; +SELECT interval 'infinity' * 0; +SELECT interval '-infinity' * 0; +SELECT interval '0 days' * 'infinity'::float; +SELECT interval '0 days' * '-infinity'::float; +SELECT interval '5 days' * 'infinity'::float; +SELECT interval '5 days' * '-infinity'::float; SELECT interval 'infinity' / 'infinity'; SELECT interval 'infinity' / '-infinity';
