Repository: genjidb/genji
Pull request #421 (2021-07-19 09:55:09)

Add more math functions 
See this spreadsheet for following progress on functions implementations: 

https://docs.google.com/spreadsheets/d/e/2PACX-1vQeXU_3ymhCOYL5mkeOVObTFhTuF6CNUogDXN-1sDn7Kb55GZtSJDquJw5PyX7JS_x0cLZCoxtou4AA/pubhtml

@asdine I think it's worth going step by step on those, taking the time to think about some decisions as we go through 👇 

In the context of those functions, I think we need to agree on: 

- Genji's function explicitly deal with their input validation, so we can produce readable error messages. Those are a part of the API after all. 
- Most of the mathematical functions are returning `NULL` when some arg is `NULL`. That's what PG / SQLite does, I see no reason from deviating from it.
- Functions that accept floats can take in integers, implicitly. Same as the above. 




codecov-commenter: # [Codecov](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) Report
> Merging [#421](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) (7a2a589) into [main](https://codecov.io/gh/genjidb/genji/commit/e556fc304847afe3d08aa2f9823e8dabe1baba64?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) (e556fc3) will **increase** coverage by `0.33%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/genjidb/genji/pull/421/graphs/tree.svg?width=650&height=150&src=pr&token=nwR1GpvtyH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb)](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb)

```diff
@@            Coverage Diff             @@
##             main     #421      +/-   ##
==========================================
+ Coverage   68.47%   68.80%   +0.33%     
==========================================
  Files          90       90              
  Lines        9590     9692     +102     
==========================================
+ Hits         6567     6669     +102     
  Misses       2299     2299              
  Partials      724      724              
```


| [Impacted Files](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb) | Coverage Δ | |
|---|---|---|
| [internal/expr/functions/math.go](https://codecov.io/gh/genjidb/genji/pull/421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb#diff-aW50ZXJuYWwvZXhwci9mdW5jdGlvbnMvbWF0aC5nbw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb). Last update [e556fc3...7a2a589](https://codecov.io/gh/genjidb/genji/pull/421?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=genjidb).


Patch:
diff --git a/cmd/genji/doc/functions.go b/cmd/genji/doc/functions.go
index 36417ac8..b18a640e 100644
--- a/cmd/genji/doc/functions.go
+++ b/cmd/genji/doc/functions.go
@@ -10,12 +10,19 @@ var packageDocs = map[string]functionDocs{
 var builtinDocs = functionDocs{
 	"pk":    "The pk() function returns the primary key for the current document",
 	"count": "Returns a count of the number of times that arg1 is not NULL in a group. The count(*) function (with no arguments) returns the total number of rows in the group.",
-	"min":   "Returns the minimum value in a group.",
-	"max":   "Returns the maximum value in a group.",
-	"sum":   "The sum function returns the sum of all values in a group.",
-	"avg":   "The avg function returns the average of all values in a group.",
+	"min":   "Returns the minimum value of the arg1 expression in a group.",
+	"max":   "Returns the maximum value of the arg1 expressein in a group.",
+	"sum":   "The sum function returns the sum of all values taken by the arg1 expression in a group.",
+	"avg":   "The avg function returns the average of all values taken by the arg1 expression in a group.",
 }
 
 var mathDocs = functionDocs{
+	"abs":   "Returns the absolute value of arg1.",
+	"acos":  "Returns the arcosine, in radiant, of arg1.",
+	"acosh": "Returns the inverse hyperbolic cosine of arg1.",
+	"asin":  "Returns the arsine, in radiant, of arg1.",
+	"asinh": "Returns the inverse hyperbolic sine of arg1.",
+	"atan":  "Returns the arctangent, in radians, of arg1.",
+	"atan2": "Returns the arctangent of arg1/arg2, using the signs of the two to determine the quadrant of the return value.",
 	"floor": "Returns the greatest integer value less than or equal to arg1.",
 }
diff --git a/internal/expr/functions/math.go b/internal/expr/functions/math.go
index a146d12e..64362dbc 100644
--- a/internal/expr/functions/math.go
+++ b/internal/expr/functions/math.go
@@ -3,6 +3,7 @@ package functions
 import (
 	"math"
 
+	"github.com/genjidb/genji/document"
 	"github.com/genjidb/genji/internal/stringutil"
 	"github.com/genjidb/genji/types"
 )
@@ -13,10 +14,17 @@ func MathFunctions() Definitions {
 }
 
 var mathFunctions = Definitions{
-	"floor": floorFunc,
+	"floor": floor,
+	"abs":   abs,
+	"acos":  acos,
+	"acosh": acosh,
+	"asin":  asin,
+	"asinh": asinh,
+	"atan":  atan,
+	"atan2": atan2,
 }
 
-var floorFunc = &ScalarDefinition{
+var floor = &ScalarDefinition{
 	name:  "floor",
 	arity: 1,
 	callFn: func(args ...types.Value) (types.Value, error) {
@@ -30,3 +38,129 @@ var floorFunc = &ScalarDefinition{
 		}
 	},
 }
+
+var abs = &ScalarDefinition{
+	name:  "abs",
+	arity: 1,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		if args[0].Type() == types.NullValue {
+			return types.NewNullValue(), nil
+		}
+		v, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil {
+			return nil, err
+		}
+		res := math.Abs(v.V().(float64))
+		if args[0].Type() == types.IntegerValue {
+			return document.CastAs(types.NewDoubleValue(res), types.IntegerValue)
+		}
+		return types.NewDoubleValue(res), nil
+	},
+}
+
+var acos = &ScalarDefinition{
+	name:  "acos",
+	arity: 1,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		if args[0].Type() == types.NullValue {
+			return types.NewNullValue(), nil
+		}
+		v, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil {
+			return nil, err
+		}
+		vv := v.V().(float64)
+		if vv > 1.0 || vv < -1.0 {
+			return nil, stringutil.Errorf("out of range, acos(arg1) expects arg1 to be within [-1, 1]")
+		}
+		res := math.Acos(vv)
+		return types.NewDoubleValue(res), nil
+	},
+}
+
+var acosh = &ScalarDefinition{
+	name:  "acosh",
+	arity: 1,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		if args[0].Type() == types.NullValue {
+			return types.NewNullValue(), nil
+		}
+		v, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil {
+			return nil, err
+		}
+		vv := v.V().(float64)
+		if vv < 1.0 {
+			return nil, stringutil.Errorf("out of range, acosh(arg1) expects arg1 >= 1")
+		}
+		res := math.Acosh(vv)
+		return types.NewDoubleValue(res), nil
+	},
+}
+
+var asin = &ScalarDefinition{
+	name:  "asin",
+	arity: 1,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		if args[0].Type() == types.NullValue {
+			return types.NewNullValue(), nil
+		}
+		v, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil {
+			return nil, err
+		}
+		vv := v.V().(float64)
+		if vv > 1.0 || vv < -1.0 {
+			return nil, stringutil.Errorf("out of range, asin(arg1) expects arg1 to be within [-1, 1]")
+		}
+		res := math.Asin(vv)
+		return types.NewDoubleValue(res), nil
+	},
+}
+
+var asinh = &ScalarDefinition{
+	name:  "asinh",
+	arity: 1,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		v, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil || v.Type() == types.NullValue {
+			return v, err
+		}
+		vv := v.V().(float64)
+		res := math.Asinh(vv)
+		return types.NewDoubleValue(res), nil
+	},
+}
+
+var atan = &ScalarDefinition{
+	name:  "atan",
+	arity: 1,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		v, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil || v.Type() == types.NullValue {
+			return v, err
+		}
+		vv := v.V().(float64)
+		res := math.Atan(vv)
+		return types.NewDoubleValue(res), nil
+	},
+}
+
+var atan2 = &ScalarDefinition{
+	name:  "atan2",
+	arity: 2,
+	callFn: func(args ...types.Value) (types.Value, error) {
+		vA, err := document.CastAs(args[0], types.DoubleValue)
+		if err != nil || vA.Type() == types.NullValue {
+			return vA, err
+		}
+		vvA := vA.V().(float64)
+		vB, err := document.CastAs(args[1], types.DoubleValue)
+		if err != nil || vB.Type() == types.NullValue {
+			return vB, err
+		}
+		vvB := vB.V().(float64)
+		res := math.Atan2(vvA, vvB)
+		return types.NewDoubleValue(res), nil
+	},
+}


Test patch:
diff --git a/cmd/genji/doc/doc_test.go b/cmd/genji/doc/doc_test.go
index 9402476a..01437284 100644
--- a/cmd/genji/doc/doc_test.go
+++ b/cmd/genji/doc/doc_test.go
@@ -2,6 +2,7 @@ package doc_test
 
 import (
 	"fmt"
+	"regexp"
 	"testing"
 
 	"github.com/genjidb/genji/cmd/genji/doc"
@@ -15,19 +16,19 @@ func TestFunctions(t *testing.T) {
 	for pkgname, pkg := range packages {
 		for fname, def := range pkg {
 			if pkgname == "" {
-				t.Run(fmt.Sprintf("%s has all its arguments mentioned", fname), func(t *testing.T) {
+				t.Run(fmt.Sprintf("%s is documented and has all its arguments mentioned", fname), func(t *testing.T) {
 					str, err := doc.DocString(fname)
 					require.NoError(t, err)
 					for i := 0; i < def.Arity(); i++ {
-						require.Contains(t, str, fmt.Sprintf("arg%d", i+1))
+						require.Contains(t, trimDocPromt(str), fmt.Sprintf("arg%d", i+1))
 					}
 				})
 			} else {
-				t.Run(fmt.Sprintf("%s.%s has all its arguments mentioned", pkgname, fname), func(t *testing.T) {
+				t.Run(fmt.Sprintf("%s.%s is documented and has all its arguments mentioned", pkgname, fname), func(t *testing.T) {
 					str, err := doc.DocString(fmt.Sprintf("%s.%s", pkgname, fname))
 					require.NoError(t, err)
 					for i := 0; i < def.Arity(); i++ {
-						require.Contains(t, str, fmt.Sprintf("arg%d", i+1))
+						require.Contains(t, trimDocPromt(str), fmt.Sprintf("arg%d", i+1))
 					}
 				})
 			}
@@ -35,6 +36,14 @@ func TestFunctions(t *testing.T) {
 	}
 }
 
+// trimDocPrompt returns the description part of the doc string, ignoring the promt.
+func trimDocPromt(str string) string {
+	// Matches the doc description, ignoring the "package.funcname:" part.
+	r := regexp.MustCompile("[^:]+:(.*)")
+	subs := r.FindStringSubmatch(str)
+	return subs[1]
+}
+
 func TestTokens(t *testing.T) {
 	for _, tok := range scanner.AllKeywords() {
 		t.Run(fmt.Sprintf("%s is documented", tok.String()), func(t *testing.T) {
diff --git a/internal/expr/functions/testdata/math_functions.sql b/internal/expr/functions/testdata/math_functions.sql
index c886d801..d6a44684 100644
--- a/internal/expr/functions/testdata/math_functions.sql
+++ b/internal/expr/functions/testdata/math_functions.sql
@@ -1,9 +1,121 @@
 -- test: math.floor
 > math.floor(2.3)
 2.0
-
 > math.floor(2)
 2
-
 ! math.floor('a')
 'floor(arg1) expects arg1 to be a number'
+
+-- test: math.abs
+> math.abs(NULL)
+NULL
+> math.abs(-2)
+2
+> math.abs(-2.0)
+2.0
+> math.abs('-2.0')
+2.0
+! math.abs('foo')
+'cannot cast "foo" as double'
+
+-- test: math.acos
+> math.acos(NULL)
+NULL
+> math.acos(1)
+0.0
+> math.acos(0.5)
+1.0471975511965976
+> math.acos('0.5')
+1.0471975511965976
+! math.acos(2)
+'out of range'
+! math.acos(-2)
+'out of range'
+! math.acos(2.2)
+'out of range'
+! math.acos(-2.2)
+'out of range'
+! math.acos('foo')
+'cannot cast "foo" as double'
+
+-- test: math.acosh
+> math.acosh(NULL)
+NULL
+> math.acosh(1)
+0.0
+> math.acosh(2)
+1.3169578969248166
+> math.acosh('2')
+1.3169578969248166
+> math.acosh(2.5)
+1.566799236972411
+! math.acosh(0)
+'out of range'
+! math.acosh(0.99999999)
+'out of range'
+! math.acosh('foo')
+'cannot cast "foo" as double'
+
+-- test: math.asin
+> math.asin(NULL)
+NULL
+> math.asin(0)
+0.0
+> math.asin(0.5)
+0.5235987755982989
+! math.asin(2)
+'out of range'
+! math.asin(-2)
+'out of range'
+! math.asin(2.2)
+'out of range'
+! math.asin(-2.2)
+'out of range'
+! math.asin('foo')
+'cannot cast "foo" as double'
+
+-- test: math.asinh
+> math.asinh(NULL)
+NULL
+> math.asinh(0)
+0.0
+> math.asinh(0.5)
+0.48121182505960347
+> math.asinh(1)
+0.881373587019543
+> math.asinh(-1)
+-0.881373587019543
+! math.asinh('foo')
+'cannot cast "foo" as double'
+
+-- test: math.atan
+> math.atan(NULL)
+NULL
+> math.atan(0)
+0.0
+> math.atan(0.5)
+0.4636476090008061
+> math.atan(1)
+0.7853981633974483
+> math.atan(-1)
+-0.7853981633974483
+! math.atan('foo')
+'cannot cast "foo" as double'
+
+-- test: math.atan2
+> math.atan2(NULL, NULL)
+NULL
+> math.atan2(1, NULL)
+NULL
+> math.atan2(NULL, 1)
+NULL
+> math.atan2(0, 0)
+0.0
+> math.atan2(1, 1)
+0.7853981633974483
+> math.atan2(1.1, 1.1)
+0.7853981633974483
+> math.atan2(1.1, -1.1)
+2.356194490192345
+! math.atan2('foo', 1)
+'cannot cast "foo" as double'