On Mon, Jun 15, 2020 at 12:41 PM Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > > Adjacent to the discussion in [0] I wanted to document the factorial() > function and expand the tests for that slightly with some edge cases. > > I noticed that the current implementation returns 1 for the factorial of > all negative numbers: > > SELECT factorial(-4); > factorial > ----------- > 1 > > While there are some advanced mathematical constructions that define > factorials for negative numbers, they certainly produce different > answers than this. > > Curiously, before the reimplementation of factorial using numeric > (04a4821adef38155b7920ba9eb83c4c3c29156f8), it returned 0 for negative > numbers, which is also not correct by any theory I could find. > > I propose to change this to error out for negative numbers.
+1. Here are some comments I see below in the .out but there's not corresponding SQL in .sql. +SELECT factorial(-4); + factorial +----------- + 1 +(1 row) + Should we also add -4! to cover both function as well as the operator? + if (num < 0) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), This looks more of ERRCODE_FEATURE_NOT_SUPPORTED esp. since factorial of negative numbers is defined but we are not supporting it. I looked at some other usages of this error code. All of them are really are out of range value errors. Otherwise the patches look good to me.
