Feature/logarithms #482

theodesp · 2018-10-19T21:18:34Z

PR adds relevant Logarithm functions:
Some examples:

mysql> select log();
ERROR 1105 (HY000): unknown error: expecting 1 or 2 arguments for calling this function, 0 received
mysql> select log(1, 2, 3);
ERROR 1105 (HY000): unknown error: expecting 1 or 2 arguments for calling this function, 3 received
mysql> select log(1, 2);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: 1
mysql> select log(-1);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: -1
mysql> select log(-1, 100);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: -1
mysql> select log(2, 10);
+-------------------+
| log(2, 10)        |
+-------------------+
| 3.321928094887362 |
+-------------------+
1 row in set (0.00 sec)

mysql> select ln(0);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: 0
mysql> select ln(-2);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: -2
mysql> select ln(20);
+-------------------+
| ln(20)            |
+-------------------+
| 2.995732273553991 |
+-------------------+
1 row in set (0.00 sec)

mysql> select log2(0);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: 0
mysql> select log2(-2);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: -2
mysql> select log2(2);
+---------+
| log2(2) |
+---------+
|       1 |
+---------+
1 row in set (0.00 sec)


mysql> select log2('5');
+-------------------+
| log2("5")         |
+-------------------+
| 2.321928094887362 |
+-------------------+
1 row in set (0.00 sec)

mysql> select log10(-10);
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: -10
mysql> select log10('-20');
ERROR 1105 (HY000): unknown error: invalid argument value for logarithm: -20
mysql> select log10(100);
+------------+
| log10(100) |
+------------+
|          2 |
+------------+
1 row in set (0.00 sec)

mysql>

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

sql/expression/function/logarithm.go

kuba-- · 2018-10-22T10:35:10Z

sql/expression/function/logarithm.go

+	ctx *sql.Context,
+	row sql.Row,
+) (interface{}, error) {
+	left, err := l.Left.Eval(ctx, row)


This common block which repeats multiple times (eval + convert) we can extract to some private function.

Sure will push a fix today

sql/expression/function/logarithm.go

sql/expression/function/registry.go

theodesp · 2018-10-22T14:52:52Z

I've consolidated Log2, Ln and Log10. @kuba-- Please take a look

kuba--

Could you also sign a request, thanks.

kuba-- · 2018-10-23T07:28:10Z

sql/expression/function/logarithm.go

+
+// IsNullable implements the sql.Expression interface.
+func (l *LogBase) IsNullable() bool {
+	return l.Child.IsNullable()


IMO Logarithm can be NULL in ranges where function y=log b(x) doesn't exist:

b or x is NULL

base is <= 0 or base == 1

x <= 0

OK I will add the check here also

BTW how can I resolve the Child value here so I can check that x <= 0?
Also as base is float there is no nil value unless I change it to a pointer.

So far I've done something like:

func (l *LogBase) IsNullable() bool { return l.base == float64(1) || l.base <= float64(0) || l.Child.IsNullable() }

I think it may work.

kuba-- · 2018-10-23T07:42:29Z

sql/expression/function/logarithm.go

+		right = args[1]
+	}
+
+	return &Log{expression.BinaryExpression{Left: args[0], Right: right}}, nil


isn't better to always set b and x instead of checking somewhere else which one is nil?

if len == 1 => left = E, right = args[0]

if len == 2 => left = args[0], right = args[1]

OK will refactor but I took this logic from
https://github.com/src-d/go-mysql-server/blob/master/sql/expression/function/ceil_round_floor.go#L155-L166

I believe, internally it's better to have everything explicit.

kuba-- · 2018-10-23T07:50:48Z

sql/expression/function/logarithm.go

+
+// Children implements the Expression interface.
+func (l *Log) Children() []sql.Expression {
+	if l.Right == nil {


IMO internally we should explicitly say - left is a base, right is argument.
If base is not specified in constructor then we set it up to E, but internally we shouldn't have hidden logic, which argument stands for what.

If we setup base to Math.E wouldn't that make difficult to detect if the request was using 1 argument instead of 2? For example when l.Right == nil we know that the request was log(%s) but now every time it would be log(%s, %s) which looks wierd.

mysql> select log(2); +---------------------------+ | log(2.718281828459045, 2) | +---------------------------+ | 0.6931471805599453 | +---------------------------+

But the header comes from the String - right?
If yes, maybe we can add extra nice formatting

Ok we can just always show the base even if its not specified. and maybe use "e"

kuba-- · 2018-10-23T07:51:00Z

sql/expression/function/logarithm.go

+}
+
+// IsNullable implements the Expression interface.
+func (l *Log) IsNullable() bool { return l.Left.IsNullable() }


see comments above

kuba-- · 2018-10-23T07:51:50Z

sql/expression/function/logarithm.go

+
+// Resolved implements the Expression interface.
+func (l *Log) Resolved() bool {
+	return l.Left.Resolved() && (l.Right == nil || l.Right.Resolved())


it's only resolvable for arguments from domain

OK, but I copied that from:

https://github.com/src-d/go-mysql-server/blob/master/sql/expression/function/ceil_round_floor.go#L258-L259

Does it mean that I don't have to provide a Resolved method?

sql/expression/function/logarithm.go

mcarmonaa · 2018-10-23T07:58:17Z

sql/expression/function/logarithm.go

+
+// LogBaseMaker returns LogBase creator functions with a specific base.
+func LogBaseMaker(base float64) func(e sql.Expression) sql.Expression {
+	b := base


can't you pass directly the argument base to the function NewLogBase in the closure?

sql/expression/function/logarithm_test.go

mcarmonaa · 2018-10-23T07:59:20Z

sql/expression/function/registry.go

@@ -3,6 +3,7 @@ package function
 import (
 	"gopkg.in/src-d/go-mysql-server.v0/sql"
 	"gopkg.in/src-d/go-mysql-server.v0/sql/expression/function/aggregation"
+	"math"


same about import style

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

theodesp · 2018-10-24T15:05:43Z

I've simplified the logic even more @kuba-- Please take a look

theodesp added 2 commits October 19, 2018 22:05

Add ln, log, log2, log10

5adf38f

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

Includ registry changes

4a5b023

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

theodesp mentioned this pull request Oct 19, 2018

Add support for LOG, LOG2, LOG10, LN #392

Closed

ajnavarro requested review from erizocosmico, kuba--, mcarmonaa and jfontan October 22, 2018 08:13

kuba-- reviewed Oct 22, 2018

View reviewed changes

kuba-- suggested changes Oct 23, 2018

View reviewed changes

mcarmonaa reviewed Oct 23, 2018

View reviewed changes

Consolidated ln, log2, log10

143c081

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

theodesp force-pushed the feature/logarithms branch from f5c4351 to 143c081 Compare October 24, 2018 15:02

Code Review fixes

b577732

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

kuba-- approved these changes Oct 24, 2018

View reviewed changes

ajnavarro approved these changes Oct 25, 2018

View reviewed changes

ajnavarro merged commit 85cd592 into src-d:master Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/logarithms #482

Feature/logarithms #482

theodesp commented Oct 19, 2018

kuba-- Oct 22, 2018

theodesp Oct 22, 2018

theodesp commented Oct 22, 2018

kuba-- left a comment

kuba-- Oct 23, 2018

theodesp Oct 23, 2018

theodesp Oct 23, 2018

kuba-- Oct 23, 2018

kuba-- Oct 23, 2018

theodesp Oct 23, 2018

kuba-- Oct 23, 2018

kuba-- Oct 23, 2018

theodesp Oct 23, 2018 •

edited

Loading

kuba-- Oct 23, 2018

theodesp Oct 24, 2018 •

edited

Loading

kuba-- Oct 23, 2018

kuba-- Oct 23, 2018

theodesp Oct 23, 2018

mcarmonaa Oct 23, 2018

mcarmonaa Oct 23, 2018

theodesp commented Oct 24, 2018

Feature/logarithms #482

Feature/logarithms #482

Conversation

theodesp commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theodesp commented Oct 22, 2018

kuba-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theodesp Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theodesp Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theodesp commented Oct 24, 2018

theodesp Oct 23, 2018 •

edited

Loading

theodesp Oct 24, 2018 •

edited

Loading