Numeric version of factorial()

Attached is a patch implementing factorial(), returning numeric. Points to
note:

1) arttype is numeric. I thought this was the best way of allowing
arbitarily large factorials, even though factorial(2^63) is a large
number. Happy to change to integers if this is overkill.
2) since we're accepting numeric arguments, the patch tests for floats. If
a numeric is passed with non-zero decimal portion, an error is raised
since (from memory) they are undefined.
3) I have not removed factorial([int2|int4|int8]), not their operator
counterparts since I didn't know what people would want done with these.
4) I haven't added any documentation but am happy to once I know if people
want int or numeric arguments.

Thanks,

Gavin

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> 2) since we're accepting numeric arguments, the patch tests for floats. If
> a numeric is passed with non-zero decimal portion, an error is raised
> since (from memory) they are undefined.

There is a standard mathematical definition for it (gamma function,
IIRC) but this is probably plenty good enough for our purposes. I would
suggest though that you reject fractions before you short-circuit for
x <= 1.

> 3) I have not removed factorial([int2|int4|int8]), not their operator
> counterparts since I didn't know what people would want done with these.

We had already decided to nuke the int2 and int4 versions, since they
overflow far too easily. I'd go with nuking int8 too and providing only
the numeric variant ...

> + int8_to_numericvar((int64)1, &one);
> +
> + ret = cmp_var(&fact, &one);

Uh, why not use const_one?

regards, tom lane

On Thu, 31 Jul 2003, Tom Lane wrote:

> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > 2) since we're accepting numeric arguments, the patch tests for floats. If
> > a numeric is passed with non-zero decimal portion, an error is raised
> > since (from memory) they are undefined.
>
> There is a standard mathematical definition for it (gamma function,
> IIRC) but this is probably plenty good enough for our purposes. I would
> suggest though that you reject fractions before you short-circuit for
> x <= 1.

Oops.

>
> > 3) I have not removed factorial([int2|int4|int8]), not their operator
> > counterparts since I didn't know what people would want done with these.
>
> We had already decided to nuke the int2 and int4 versions, since they
> overflow far too easily. I'd go with nuking int8 too and providing only
> the numeric variant ...

What are your feelings about numeric argument vs. int4/int8 arguments?

>
> > + int8_to_numericvar((int64)1, &one);
> > +
> > + ret = cmp_var(&fact, &one);
>
> Uh, why not use const_one?

Umm.. didn't notice it :-).

Thanks,

Gavin

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> What are your feelings about numeric argument vs. int4/int8 arguments?

Actually I think it'd be fine to take int8. We'd not be able to cope
with any larger input anyway, and the inner loop could be noticeably
faster if the control logic just deals with int.

We could leave the factorial(numeric) case open for a future
implementation that uses gamma, if anyone gets hot to do it.

regards, tom lane

On Thu, 31 Jul 2003, Tom Lane wrote:

> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > What are your feelings about numeric argument vs. int4/int8 arguments?
>
> Actually I think it'd be fine to take int8. We'd not be able to cope
> with any larger input anyway, and the inner loop could be noticeably
> faster if the control logic just deals with int.
>
> We could leave the factorial(numeric) case open for a future
> implementation that uses gamma, if anyone gets hot to do it.
>

Attached is a revised patch based on your Tom's comments. It removes
int[248]fac(), modifies regression tests (which referenced int4fac()), and
implements a much cleaned numeric_fac().

> regards, tom lane
>

Thanks,

Gavin

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Gavin Sherry wrote:
> On Thu, 31 Jul 2003, Tom Lane wrote:
>
> > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > What are your feelings about numeric argument vs. int4/int8 arguments?
> >
> > Actually I think it'd be fine to take int8. We'd not be able to cope
> > with any larger input anyway, and the inner loop could be noticeably
> > faster if the control logic just deals with int.
> >
> > We could leave the factorial(numeric) case open for a future
> > implementation that uses gamma, if anyone gets hot to do it.
> >
>
> Attached is a revised patch based on your Tom's comments. It removes
> int[248]fac(), modifies regression tests (which referenced int4fac()), and
> implements a much cleaned numeric_fac().
>
> > regards, tom lane
> >
>
> Thanks,
>
> Gavin

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Patch attached and applied. Thanks.

I adjusted the oid so prevent a duplicate, and adjusted the regression
test to remove comments on now non-existant functions.

I also tested the output and it looks good:

test=> select numeric_fac(10);
numeric_fac
-------------
3628800
(1 row)

test=> select numeric_fac(1);
numeric_fac
-------------
1
(1 row)

test=> select numeric_fac(2);
numeric_fac
-------------
2
(1 row)

test=> select numeric_fac(4);
numeric_fac
-------------
24
(1 row)

test=> select numeric_fac(3);
numeric_fac
-------------
6
(1 row)

I will check the docs now.

---------------------------------------------------------------------------

Gavin Sherry wrote:
> Attached is a patch implementing factorial(), returning numeric. Points to
> note:
>
> 1) arttype is numeric. I thought this was the best way of allowing
> arbitarily large factorials, even though factorial(2^63) is a large
> number. Happy to change to integers if this is overkill.
> 2) since we're accepting numeric arguments, the patch tests for floats. If
> a numeric is passed with non-zero decimal portion, an error is raised
> since (from memory) they are undefined.
> 3) I have not removed factorial([int2|int4|int8]), not their operator
> counterparts since I didn't know what people would want done with these.
> 4) I haven't added any documentation but am happy to once I know if people
> want int or numeric arguments.

> Attached is a revised patch based on your Tom's comments. It removes
> int[248]fac(), modifies regression tests (which referenced int4fac()),
> and implements a much cleaned numeric_fac().

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

I had to remove the factorial example from the casting docs because
factorial is now defined only for numeric. Would someone take the
attached example and make a new one with a different operator?

Thanks.

---------------------------------------------------------------------------

Gavin Sherry wrote:
> Attached is a patch implementing factorial(), returning numeric. Points to
> note:
>
> 1) arttype is numeric. I thought this was the best way of allowing
> arbitarily large factorials, even though factorial(2^63) is a large
> number. Happy to change to integers if this is overkill.
> 2) since we're accepting numeric arguments, the patch tests for floats. If
> a numeric is passed with non-zero decimal portion, an error is raised
> since (from memory) they are undefined.
> 3) I have not removed factorial([int2|int4|int8]), not their operator
> counterparts since I didn't know what people would want done with these.
> 4) I haven't added any documentation but am happy to once I know if people
> want int or numeric arguments.
>
> Thanks,
>
> Gavin

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I had to remove the factorial example from the casting docs because
> factorial is now defined only for numeric. Would someone take the
> attached example and make a new one with a different operator?

Done.

regards, tom lane