Implement CONVERT expression and cast expressions in comparisons. #103

mcarmonaa · 2018-03-14T14:26:01Z

Closes #92

erizocosmico · 2018-03-14T15:20:23Z

sql/analyzer/rules.go

@@ -411,3 +412,71 @@ func pushdown(a *Analyzer, n sql.Node) (sql.Node, error) {
 		return node, nil
 	})
 }
+
+func castCompares(a *Analyzer, n sql.Node) (sql.Node, error) {


What do you think about putting this logic inside the Comparison Compare method instead?

Compare has access to the types and it could handle them itself using the Convert method of the types. The less rules we need, the better.

The rule that I'm missing here is one to cast literals in comparisons when the types on left and right are not equals.

Literals are like any other Expression and they have certain type. The rule can't be cast always the literal to the other type, at least mysql doesn't work in this way.

For example, when the types at both side of the comparison are different, mysql cast to numbers if some of the type of them is a number, independently if there is a literal or not.

As you can see in this example, mysql cast the text values from the num column to numbers, (the casting of the text values evals to 0 because they don't represent numbers):

MySQL [testing]> select * from tabletest; +----+------+------+ | id | name | num | +----+------+------+ | 1 | a | 1 | | 2 | b | 2 | | 3 | c | 3 | | 4 | d | 4 | +----+------+------+ 4 rows in set (0.00 sec) MySQL [testing]> select * from tabletest where name > 0; Empty set (0.00 sec) MySQL [testing]> select * from tabletest where name < 0; Empty set (0.01 sec) MySQL [testing]> select * from tabletest where name = 0; +----+------+------+ | id | name | num | +----+------+------+ | 1 | a | 1 | | 2 | b | 2 | | 3 | c | 3 | | 4 | d | 4 | +----+------+------+ 4 rows in set (0.00 sec)

erizocosmico · 2018-03-14T15:21:18Z

sql/expression/comparison.go

 }

 // Eval implements the Expression interface.
 func (gte GreaterThanOrEqual) Eval(
 	session sql.Session,
 	row sql.Row,
 ) (interface{}, error) {
-	a, err := gte.Left.Eval(session, row)
+	a, err := gte.BinaryExpression.Left.Eval(session, row)


why add the BinaryExpression here?

Because of the Left() method in the interface, it shadows the Left field of the embedded BinaryExpression.

erizocosmico · 2018-03-14T15:22:13Z

sql/expression/comparison.go

+	IsComparison() bool
+	Left() sql.Expression
+	Right() sql.Expression
+	SetLeft(sql.Expression)


nodes should be immutable. If it needs to change, a new node should be created

erizocosmico · 2018-03-14T15:22:44Z

sql/expression/convert.go

+
+// TransformUp implements the Expression interface.
+func (c *Convert) TransformUp(f func(sql.Expression) (sql.Expression, error)) (sql.Expression, error) {
+	child, err := c.UnaryExpression.Child.TransformUp(f)


UnaryExpression here is redundant

erizocosmico · 2018-03-14T15:22:58Z

sql/expression/convert.go

+
+// Eval implements the Expression interface.
+func (c *Convert) Eval(session sql.Session, row sql.Row) (interface{}, error) {
+	val, err := c.UnaryExpression.Child.Eval(session, row)


UnaryExpression here is redundant

ajnavarro · 2018-03-15T09:51:44Z

engine_test.go

+		},
+	)
+
+	testQuery(t, e,


This test and the above one are the same

ajnavarro · 2018-03-15T10:14:29Z

sql/analyzer/rules.go

@@ -411,3 +412,71 @@ func pushdown(a *Analyzer, n sql.Node) (sql.Node, error) {
 		return node, nil
 	})
 }
+
+func castCompares(a *Analyzer, n sql.Node) (sql.Node, error) {


The rule that I'm missing here is one to cast literals in comparisons when the types on left and right are not equals.

ajnavarro · 2018-03-15T10:15:28Z

sql/analyzer/rules.go

+	})
+}
+
+func isNumber(t sql.Type) bool {


Maybe this logic to check types should be in types.go file

ajnavarro · 2018-03-15T10:16:47Z

sql/analyzer/rules_test.go

+			expected: plan.NewProject(
+				[]sql.Expression{
+					expression.NewGreaterThanOrEqual(
+						expression.NewConvert(


The auto casting should be always only on the literal side I think

As explained up there, the cast can't be just for literals and should be for both expressions because of the "types" the convert expression accepts.

Anyway, I'm moving all the logic to Comparison expression and removing the rule as @erizocosmico suggested.

ajnavarro · 2018-03-15T10:19:05Z

sql/expression/comparison.go

+	SetRight(sql.Expression)
+}
+
+type comparison struct {


this should be public, to be able to check if the expression is a Comparison on rules.

That check is done by the interface Comparer, because comparison is just used as an embedded type in the real comparison expressions.

If you try a type assertion on a interface by the embedded type of the type, it doesn't work.
See: https://play.golang.org/p/NJHOB7G27mc

mcarmonaa · 2018-03-15T14:15:21Z

@erizocosmico @ajnavarro I removed the rule and moved the casting types logic to the comparison expressions

erizocosmico · 2018-03-15T15:23:49Z

sql/expression/comparison.go

@@ -3,73 +3,165 @@ package expression
 import (
 	"regexp"

+	errors "gopkg.in/src-d/go-errors.v0"


we should be using errors v1

Should I change it across all the repo?

Change only the added ones, I opened an issue for the rest: #115

erizocosmico · 2018-03-15T15:24:02Z

sql/expression/comparison.go

+	Right() sql.Expression
+}
+
+var ErrNilOperand = errors.NewKind("nil operand found in comparison")


comment missing for this

erizocosmico · 2018-03-15T15:30:08Z

sql/expression/convert.go

+}
+
+// ConvetValue cast the value of val to castTo throw the Convert expression rules.
+func ConvertValue(val interface{}, castTo string) (interface{}, error) {


is this used outside this package? If it's not, I'd keep it lowercase

erizocosmico · 2018-03-15T15:31:31Z

sql/expression/convert.go

+
+const (
+	// ConvertTo represents those conversion types Convert accepts.
+	ConvertToBinary   = "binary"


are these constants used outside this package? If not, I'd keep them lowercase. And if so, I would make them integers, the take less space and the string is not really needed. They're only needed as an enum

Those are the string go-vitess' parser returns, I can change it to enums, but I'd have to create two maps kept in memory to translate strings->int and int->strings and make the logic a little bit complex.

They are only used inside the expression package, but I think it's ok to have them public if at some point we need to create Convert expressions in a rule or something like that.

Do you think it will make a really improve the enum thing?

Then I think the enum is not worth it, too much hassle for so little benefit. LGTM as is, then.

erizocosmico

LGTM, good job!

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

…rand Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa requested review from jfontan, ajnavarro and erizocosmico March 14, 2018 14:26

erizocosmico suggested changes Mar 14, 2018

View reviewed changes

erizocosmico mentioned this pull request Mar 15, 2018

it: commits that appear on more than one reference src-d/gitbase#179

Merged

ajnavarro suggested changes Mar 15, 2018

View reviewed changes

mcarmonaa force-pushed the fix/cast-literals branch from 094658c to 2508134 Compare March 15, 2018 14:08

ajnavarro approved these changes Mar 15, 2018

View reviewed changes

erizocosmico suggested changes Mar 15, 2018

View reviewed changes

jfontan approved these changes Mar 16, 2018

View reviewed changes

erizocosmico approved these changes Mar 16, 2018

View reviewed changes

mcarmonaa force-pushed the fix/cast-literals branch from 50219df to e5e4cac Compare March 16, 2018 11:02

mcarmonaa added 9 commits March 16, 2018 12:13

add Convert expression

65ce944

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

create an analyzer rule to cast expressions in a comparison

8e3d0b8

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

remove duplicated test in engine_test.go

9cb3a04

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

removed cast_compares rule from the analyzer

14fb95d

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

add public funtions to type.go for the kind of the type

0f7eedc

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

refactor Convert expression

9eb954c

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

add logic to Comparison expressions to cast types

ece7c90

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

make convertValue private, use go-errors.v1, add comment to ErrNilOpe…

7288fff

…rand Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

implement String() instead of Name() in Convert expression

2f1992d

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa force-pushed the fix/cast-literals branch from e5e4cac to 2f1992d Compare March 16, 2018 11:14

mcarmonaa merged commit 87960db into src-d:master Mar 16, 2018

mcarmonaa deleted the fix/cast-literals branch March 16, 2018 11:22

Implement CONVERT expression and cast expressions in comparisons. #103

Implement CONVERT expression and cast expressions in comparisons. #103

Uh oh!

Conversation

mcarmonaa commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarmonaa Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarmonaa commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erizocosmico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcarmonaa commented Mar 14, 2018 •

edited

Loading

mcarmonaa Mar 15, 2018 •

edited

Loading