Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parentheses behaviour #131

Merged
merged 4 commits into from
Jul 22, 2020
Merged

Fix parentheses behaviour #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1b5ba3
Add parentheses expression
asdine Jul 19, 2020
89a5913
Update tests
asdine Jul 19, 2020
dca1f89
Improve docstring
asdine Jul 19, 2020
b425b13
Fix InsertStmt tests
asdine Jul 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions sql/parser/expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,14 @@ func (p *Parser) parseUnaryExpr() (expr.Expr, error) {
p.Unscan()
return p.parseExprList(scanner.LSBRACKET, scanner.RSBRACKET)
case scanner.LPAREN:
p.Unscan()
return p.parseExprList(scanner.LPAREN, scanner.RPAREN)
e, _, err := p.ParseExpr()
if err != nil {
return nil, err
}
if tok, pos, lit := p.ScanIgnoreWhitespace(); tok != scanner.RPAREN {
return nil, newParseError(scanner.Tokstr(tok, lit), []string{")"}, pos)
}
return expr.Parentheses{E: e}, nil
default:
return nil, newParseError(scanner.Tokstr(tok, lit), []string{"identifier", "string", "number", "bool"}, pos)
}
Expand Down
33 changes: 21 additions & 12 deletions sql/parser/expr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,26 +87,35 @@ func TestParserExpr(t *testing.T) {
{"bad document: missing right bracket", `{a: 1`, nil, true},
{"bad document: missing colon", `{a: 1, 'b'}`, nil, true},

// list of expressions
{"list with parentheses: empty", "()", expr.LiteralExprList(nil), false},
{"list with parentheses: values", `(1, true, {a: 1}, a.b.c, (-1), [-1])`,
expr.LiteralExprList{
expr.IntegerValue(1),
expr.BoolValue(true),
expr.KVPairs{expr.KVPair{K: "a", V: expr.IntegerValue(1)}},
expr.FieldSelector{"a", "b", "c"},
expr.LiteralExprList{expr.IntegerValue(-1)},
expr.LiteralExprList{expr.IntegerValue(-1)},
// parentheses
{"parentheses: empty", "()", nil, true},
{"parentheses: values", `(1)`,
expr.Parentheses{
E: expr.IntegerValue(1),
}, false},
{"parentheses: expr", `(1 + true * (4 + 3))`,
expr.Parentheses{
E: expr.Add(
expr.IntegerValue(1),
expr.Mul(
expr.BoolValue(true),
expr.Parentheses{
E: expr.Add(
expr.IntegerValue(4),
expr.IntegerValue(3),
),
},
),
),
}, false},
{"list with parentheses: missing parenthese", `(1, true, {a: 1}, a.b.c, (-1)`, nil, true},
{"list with brackets: empty", "[]", expr.LiteralExprList(nil), false},
{"list with brackets: values", `[1, true, {a: 1}, a.b.c, (-1), [-1]]`,
expr.LiteralExprList{
expr.IntegerValue(1),
expr.BoolValue(true),
expr.KVPairs{expr.KVPair{K: "a", V: expr.IntegerValue(1)}},
expr.FieldSelector{"a", "b", "c"},
expr.LiteralExprList{expr.IntegerValue(-1)},
expr.Parentheses{E: expr.IntegerValue(-1)},
expr.LiteralExprList{expr.IntegerValue(-1)},
}, false},
{"list with brackets: missing bracket", `[1, true, {a: 1}, a.b.c, (-1), [-1]`, nil, true},
Expand Down
13 changes: 13 additions & 0 deletions sql/query/expr/expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,16 @@ type Operator interface {
SetRightHandExpr(Expr)
Token() scanner.Token
}

// Parentheses is a special expression which turns
// any sub-expression as unary.
// It hides the underlying operator, if any, from the parser
// so that it doesn't get reordered by precedence.
Comment on lines +125 to +126
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that mean exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SQL, operators are ordered by precedence, for example, * has higher precedence than +, which means it must be evaluated first:

3 + 4 * 2
# 4 * 2 is evaluated first
3 + 8
# then + is evaluated
11

If you want to avoid that, you can use parentheses

(3 + 4) * 2
# 3 + 4 is evaluated first
7 * 2
# then * is evaluated
14

This behavior is done in the parser, when parsing the expression: https://github.com/genjidb/genji/blob/454cc983db979394c310a5a5df0e1180fb0f1447/sql/parser/expr.go#L30-L78

This function builds a tree based on operator precedence.
To avoid an expression in parentheses to have its precedence analyzed and taken into account by that function (these two lines) we "hide" it by wrapping it in a type that doesn't implement the Operator interface so that in the eye of these two lines, that expression is unary, not binary.

type Parentheses struct {
E Expr
}

// Eval calls the underlying expression Eval method.
func (p Parentheses) Eval(es EvalStack) (document.Value, error) {
return p.E.Eval(es)
}
4 changes: 2 additions & 2 deletions sql/query/insert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ func TestInsertStmt(t *testing.T) {
{"Values / Positional Params", "INSERT INTO test (a, b, c) VALUES (?, 'e', ?)", false, `{"pk()":1,"a":"d","b":"e","c":"f"}`, []interface{}{"d", "f"}},
{"Values / Named Params", "INSERT INTO test (a, b, c) VALUES ($d, 'e', $f)", false, `{"pk()":1,"a":"d","b":"e","c":"f"}`, []interface{}{sql.Named("f", "f"), sql.Named("d", "d")}},
{"Values / Invalid params", "INSERT INTO test (a, b, c) VALUES ('d', ?)", true, "", []interface{}{'e'}},
{"Values / List", `INSERT INTO test (a, b, c) VALUES ("a", 'b', (1, 2, 3))`, false, `{"pk()":1,"a":"a","b":"b","c":[1,2,3]}`, nil},
{"Values / List", `INSERT INTO test (a, b, c) VALUES ("a", 'b', [1, 2, 3])`, false, `{"pk()":1,"a":"a","b":"b","c":[1,2,3]}`, nil},
{"Documents", "INSERT INTO test VALUES {a: 'a', b: 2.3, c: 1 = 1}", false, `{"pk()":1,"a":"a","b":2.3,"c":true}`, nil},
{"Documents / Positional Params", "INSERT INTO test VALUES {a: ?, b: 2.3, c: ?}", false, `{"pk()":1,"a":"a","b":2.3,"c":true}`, []interface{}{"a", true}},
{"Documents / Named Params", "INSERT INTO test VALUES {a: $a, b: 2.3, c: $c}", false, `{"pk()":1,"a":1,"b":2.3,"c":true}`, []interface{}{sql.Named("c", true), sql.Named("a", 1)}},
{"Documents / List ", "INSERT INTO test VALUES {a: (1, 2, 3)}", false, `{"pk()":1,"a":[1,2,3]}`, nil},
{"Documents / List ", "INSERT INTO test VALUES {a: [1, 2, 3]}", false, `{"pk()":1,"a":[1,2,3]}`, nil},
{"Documents / strings", `INSERT INTO test VALUES {'a': 'a', b: 2.3}`, false, `{"pk()":1,"a":"a","b":2.3}`, nil},
{"Documents / double quotes", `INSERT INTO test VALUES {"a": "b"}`, false, `{"pk()":1,"a":"b"}`, nil},
}
Expand Down