Hi Jeevan, Nice feature; I tested it locally and it works correctly. NULL handling, parallel aggregate (combine), and the moving-window. Fallback to recalculation are all fine, no correctness bug was found. I only have the following point with a short description.
*There is no fast path for the common case; it always goes through Numeric:* For int2/int4/int8/float4/float8, every row undergoes a full arbitrary-precision Numeric conversion plus numeric_mul, even when the running product would easily fit in int64/int128 for most rows. This file already has a pattern for exactly this problem (int8 SUM uses int128 internally, only promoting to numeric on real overflow). I think PRODUCT(int4)/PRODUCT(int2) over a large table will be much slower per row than SUM for the same data, because of this. So, if possible, consider using the same native-then-promote-on-overflow approach here. Thanks, Vaibhav Dalvi EnterpriseDB On Fri, Jun 26, 2026 at 11:24 AM Jeevan Chalke < [email protected]> wrote: > Hello, > > CFbot flagged this for a rebase. The conflicts were due to the catalog > version bump, so I've dropped it here and noted in the commit message > that the committer should bump catversion at commit time to avoid > recurring conflicts. > > Also added tests as suggested by Jim. > > Thanks > > On Tue, Jun 23, 2026 at 5:26 PM Jeevan Chalke < > [email protected]> wrote: > >> >> >> On Tue, Jun 23, 2026 at 4:32 PM Jim Jones <[email protected]> >> wrote: >> >>> Hi Jeevan >>> >>> On 23/06/2026 10:37, Dean Rasheed wrote: >>> > On Tue, 23 Jun 2026 at 08:49, Jeevan Chalke >>> > <[email protected]> wrote: >>> >> PRODUCT() returns the product of all non-null input values. It is >>> defined for >>> >> int2, int4, int8, float4, float8 and numeric input, and always >>> returns numeric. >>> > I don't think that you need to define it for all those types. I >>> > suspect that you could just define it for numeric and float8, and let >>> > implicit casting do the rest. >>> >>> +1 >>> >>> I've tested the patch in many different scenarios and all results look >>> fine -- valgrind also didn't report anything :) >>> >>> The test coverage is comprehensive! For the sake of completeness I'd add >>> numeric tests for NaN and Infitinty with positive numeric values in the >>> set, e.g: >>> >>> postgres=# WITH j (v) AS (VALUES >>> ('NaN'::numeric),('Infinity'::numeric),(3.14)) >>> SELECT product(v) FROM j; >>> product >>> --------- >>> NaN >>> (1 row) >>> >>> Other than that and the point mentioned by Dean I have nothing to add at >>> this point. >>> >> >> Thanks, Jim, for the thorough testing. >> >> I'll include that test case in the next version of the patch. >> >> >> >>> >>> Thanks for the patch. >>> >>> Best, Jim >>> >> >> >> -- >> *Jeevan Chalke* >> *Senior Principal Engineer, Engineering Manager* >> *Product Development* >> >> enterprisedb.com <https://www.enterprisedb.com> >> > > > -- > *Jeevan Chalke* > *Senior Principal Engineer, Engineering Manager* > *Product Development* > > enterprisedb.com <https://www.enterprisedb.com> >
