evalengine: Implement integer division and modulo #12656

dbussink · 2023-03-18T10:42:38Z

As follow up on #12369, this implements additional missing arithmetic support for DIV, % (and the MOD alias).

It also adds support to the compiler for NOT, AND, OR and XOR which was missing as well.

Related Issue(s)

Implements part of the items listed in #9647 as well.

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Signed-off-by: Dirkjan Bussink <[email protected]>

This adds the NOT and generic logical operations to the compiler and fixes a number of existing bugs in the evalengine. Specifically NOT is currently broken as we don't translate it properly. Some main issues are that we need to ensure lazy evaluation for the logical operations but also needing it for arithmetic as well. All cases where we've been pushing the boolean singleton value need to be fixed as well in the compiler, because we inline update things in arithmetic operations and we'd update the singleton value before. It also needs to split parsing into JSON from using an argument as a partial JSON value. This specifically is needed for CAST() with an input string where it should parse it. Additionally, convering a JSON boolean to numeric needs to create a floating point value, not an integer one. Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink · 2023-03-19T12:30:10Z

go/vt/vtgate/evalengine/translate_card.go

@@ -134,6 +134,8 @@ func (ast *astCompiler) cardExpr(expr Expr) error {
 		return ast.cardUnary(expr.Inner)
 	case *BitwiseNotExpr:
 		return ast.cardUnary(expr.Inner)
+	case *NotExpr:
+		return ast.cardUnary(expr.Inner)


@systay I've included the fix for NOT here and also fixed up the types and updated it all with additional tests. It also adds the other logical operations like AND to the compiler with tests which we were missing as well.

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink · 2023-03-20T10:00:32Z

go/vt/vtgate/evalengine/compiler_arithmetic.go

@@ -92,14 +100,15 @@ func (c *compiler) compileArithmeticAdd(left, right Expr) (ctype, error) {
 	if err != nil {
 		return ctype{}, err
 	}
+	skip1 := c.compileNullCheck1(lt)


These are needed because arithmetic operations short circuit if the left hand side has a nil and it doesn't evaluable the right hand side then.

dbussink · 2023-03-20T10:01:12Z

go/vt/vtgate/evalengine/compiler_arithmetic.go


 	rt, err := c.compileExpr(right)
 	if err != nil {
 		return ctype{}, err
 	}

 	swap := false
-	skip := c.compileNullCheck2(lt, rt)
+	skip2 := c.compileNullCheck2(lt, rt)


Still need compileNullCheck2 because at this point the left hand side value is already pushed onto the stack and if we'd use compileNullCheck1 just for the right hand side, we'd leave that lingering on the stack risking future stack overflows.

dbussink · 2023-03-20T10:03:03Z

go/vt/vtgate/evalengine/compiler_json.go

@@ -66,14 +66,34 @@ func (c *compiler) compileToJSON(doct ctype, offset int) (ctype, error) {
 	return ctype{Type: sqltypes.TypeJSON, Col: collationJSON}, nil
 }

+func (c *compiler) compileArgToJSON(doct ctype, offset int) (ctype, error) {


We need separate handling for an argument to say JSON_OBJECT to a partial JSON object. For example, a JSON string on its own is not a valid JSON object but for JSON_OBJECT we need to allow partial JSON types so we can instantiate the right one.

On the other hand, usages like CAST('' as json) evaluate if the string is a full JSON object and don't allow partial things like strings so it's different. So hence a new function here.

dbussink · 2023-03-20T10:03:58Z

go/vt/vtgate/evalengine/eval_numeric.go

@@ -111,9 +111,9 @@ func evalToNumeric(e eval) evalNumeric {
 	case *evalJSON:
 		switch e.Type() {
 		case json.TypeTrue:
-			return newEvalBool(true)
+			return &evalFloat{f: 1.0}


Turns out that converting a JSON boolean turns it into a 1.0 or 0.0 float value and not an int64 1 or 0. This was not covered by tests before.

dbussink · 2023-03-20T10:05:04Z

go/vt/vtgate/evalengine/expr_logical.go

@@ -100,55 +100,100 @@ func (left boolean) not() boolean {
 	}
 }

-func (left boolean) and(right boolean) boolean {
+func opAnd(le, re Expr, env *ExpressionEnv) (boolean, error) {


These are all changed so we can also do lazy evaluation if the left hand side already gives a definitive answer. If the left hand side of an AND is already explicitly false, we never evaluable the right hand side (which matches MySQL behavior but was never tested before).

dbussink · 2023-03-20T10:05:27Z

go/vt/vtgate/evalengine/expr_logical.go

@@ -162,21 +207,18 @@ func (n *NotExpr) eval(env *ExpressionEnv) (eval, error) {

 func (n *NotExpr) typeof(env *ExpressionEnv) (sqltypes.Type, typeFlag) {
 	_, flags := n.Inner.typeof(env)
-	return sqltypes.Uint64, flags
+	return sqltypes.Int64, flags | flagIsBoolean


These were just plain wrong before, it's actually an Int64 empirically when testing against MySQL.

dbussink · 2023-03-20T10:08:48Z

go/vt/vtgate/evalengine/testcases/cases.go

@@ -872,5 +897,10 @@ func InStatement(yield Query) {
 		yield(fmt.Sprintf("%s IN (%s, %s)", inputs[2], inputs[1], inputs[0]), nil)
 		yield(fmt.Sprintf("%s IN (%s, %s)", inputs[1], inputs[0], inputs[2]), nil)
 		yield(fmt.Sprintf("%s IN (%s, %s, %s)", inputs[0], inputs[1], inputs[2], inputs[0]), nil)
+
+		yield(fmt.Sprintf("%s NOT IN (%s, %s)", inputs[0], inputs[1], inputs[2]), nil)


There was no coverage yet of NOT IN so added that here as well.

dbussink · 2023-03-20T10:09:12Z

go/vt/vtgate/evalengine/testcases/inputs.go

+	"cast('{}' as json)", "cast('[]' as json)",
+	"cast('null' as json)", "cast('true' as json)", "cast('false' as json)",
+	"cast('1' as json)", "cast('1.1' as json)", "cast('-1.1' as json)",
+	"cast('\"foo\"' as json)", "cast('invalid' as json)",


These all test various JSON edge cases in casting and operations that drive a bunch of the changes here.

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink requested review from harshit-gangal, systay, frouioui and GuptaManan100 as code owners March 18, 2023 10:42

dbussink added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Evalengine changes to the evaluation engine labels Mar 19, 2023

dbussink added 2 commits March 19, 2023 13:28

evalengine: Implement integer division and modulo

00ccf1e

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink force-pushed the implement-div-and-mod branch from 57adadf to 7b83dbb Compare March 19, 2023 12:28

dbussink commented Mar 19, 2023

View reviewed changes

Fix tests

c6cef55

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink commented Mar 20, 2023

View reviewed changes

evalengine/compiler: Use helpers where appropriate

44f2a21

Signed-off-by: Dirkjan Bussink <[email protected]>

systay approved these changes Mar 21, 2023

View reviewed changes

GuptaManan100 approved these changes Mar 21, 2023

View reviewed changes

dbussink merged commit 7eb4f35 into vitessio:main Mar 21, 2023

dbussink deleted the implement-div-and-mod branch March 21, 2023 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evalengine: Implement integer division and modulo #12656

evalengine: Implement integer division and modulo #12656

dbussink commented Mar 18, 2023 •

edited

Loading

dbussink Mar 19, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

dbussink Mar 20, 2023

evalengine: Implement integer division and modulo #12656

evalengine: Implement integer division and modulo #12656

Conversation

dbussink commented Mar 18, 2023 • edited Loading

Related Issue(s)

Checklist

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

dbussink commented Mar 18, 2023 •

edited

Loading