Hi Joseph,
thanks for addressing comments.
On Sat, Apr 1, 2023 at 10:53 PM Joseph Koshakow <koshy44@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