Skip to content

Commit

Permalink
fix(embedded/sql): correctly handle logical operator precedence (NOT,…
Browse files Browse the repository at this point in the history 
… AND, OR)

Signed-off-by: Stefano Scafiti <[email protected]>
  • Loading branch information
@ostafen
ostafen committed Dec 10, 2024
1 parent 344275d commit b7ff0e6
Show file tree
Hide file tree
Showing 7 changed files with 312 additions and 254 deletions.
4 changes: 2 additions & 2 deletions embedded/document/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1175,14 +1175,14 @@ func generateSQLFilteringExpression(expressions []*protomodel.QueryExpression, t
if i == 0 {
innerExp = fieldExp
} else {
innerExp = sql.NewBinBoolExp(sql.AND, innerExp, fieldExp)
innerExp = sql.NewBinBoolExp(sql.And, innerExp, fieldExp)
}
}

if i == 0 {
outerExp = innerExp
} else {
outerExp = sql.NewBinBoolExp(sql.OR, outerExp, innerExp)
outerExp = sql.NewBinBoolExp(sql.Or, outerExp, innerExp)
}
}

Expand Down
13 changes: 2 additions & 11 deletions embedded/sql/parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ var reservedWords = map[string]int{
"AS": AS,
"ASC": ASC,
"DESC": DESC,
"AND": AND,
"OR": OR,
"NOT": NOT,
"LIKE": LIKE,
"EXISTS": EXISTS,
Expand Down Expand Up @@ -157,11 +159,6 @@ var cmpOps = map[string]CmpOperator{
">=": GE,
}

var logicOps = map[string]LogicOperator{
"AND": AND,
"OR": OR,
}

var ErrEitherNamedOrUnnamedParams = errors.New("either named or unnamed params")
var ErrEitherPosOrNonPosParams = errors.New("either positional or non-positional named params")
var ErrInvalidPositionalParameter = errors.New("invalid positional parameter")
Expand Down Expand Up @@ -348,12 +345,6 @@ func (l *lexer) Lex(lval *yySymType) int {
return BOOLEAN
}

lop, ok := logicOps[tid]
if ok {
lval.logicOp = lop
return LOP
}

afn, ok := aggregateFns[tid]
if ok {
lval.aggFn = afn
Expand Down
75 changes: 60 additions & 15 deletions embedded/sql/parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ func TestInsertIntoStmt(t *testing.T) {
},
targets: nil,
where: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{op: GE, left: &ColSelector{col: "balance"}, right: &Integer{val: 0}},
right: &CmpBoolExp{op: EQ, left: &ColSelector{col: "deleted_at"}, right: &NullValue{t: AnyType}},
},
Expand Down Expand Up @@ -1009,9 +1009,9 @@ func TestSelectStmt(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: AND,
op: And,
left: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{
op: EQ,
left: &ColSelector{
Expand Down Expand Up @@ -1208,7 +1208,7 @@ func TestSelectStmt(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{
op: GE,
left: &ColSelector{
Expand Down Expand Up @@ -1421,7 +1421,7 @@ func TestParseExp(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: AND,
op: And,
left: &NotBoolExp{
exp: &CmpBoolExp{
op: GT,
Expand Down Expand Up @@ -1453,7 +1453,7 @@ func TestParseExp(t *testing.T) {
ds: &tableRef{table: "table1"},
where: &NotBoolExp{
exp: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{
op: GT,
left: &ColSelector{
Expand Down Expand Up @@ -1483,7 +1483,7 @@ func TestParseExp(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: OR,
op: Or,
left: &NotBoolExp{
exp: &ColSelector{col: "active"},
},
Expand All @@ -1502,7 +1502,7 @@ func TestParseExp(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{
op: GT,
left: &ColSelector{
Expand Down Expand Up @@ -1572,9 +1572,9 @@ func TestParseExp(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: OR,
op: Or,
left: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{
op: GT,
left: &ColSelector{
Expand Down Expand Up @@ -1711,7 +1711,7 @@ func TestParseExp(t *testing.T) {
whenThen: []whenThenClause{
{
when: &BinBoolExp{
op: OR,
op: Or,
left: &ColSelector{col: "is_deleted"},
right: &ColSelector{col: "is_expired"},
},
Expand All @@ -1736,7 +1736,7 @@ func TestParseExp(t *testing.T) {
whenThen: []whenThenClause{
{
when: &BinBoolExp{
op: OR,
op: Or,
left: &ColSelector{col: "is_deleted"},
right: &ColSelector{col: "is_expired"},
},
Expand Down Expand Up @@ -1798,7 +1798,7 @@ func TestParseExp(t *testing.T) {
},
{
when: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{op: GE, left: &ColSelector{col: "stock"}, right: &Integer{10}},
right: &CmpBoolExp{op: LE, left: &ColSelector{col: "stock"}, right: &Integer{50}},
},
Expand Down Expand Up @@ -1898,7 +1898,7 @@ func TestMultiLineStmts(t *testing.T) {
},
ds: &tableRef{table: "table1"},
where: &BinBoolExp{
op: AND,
op: And,
left: &CmpBoolExp{
op: GE,
left: &ColSelector{
Expand Down Expand Up @@ -2033,7 +2033,7 @@ func TestGrantRevokeStmt(t *testing.T) {
}
}

func TestExprString(t *testing.T) {
func TestExpString(t *testing.T) {
exps := []string{
"(1 + 1) / (2 * 5 - 10) % 2",
"@param LIKE 'pattern'",
Expand All @@ -2043,6 +2043,8 @@ func TestExprString(t *testing.T) {
"CASE WHEN in_stock THEN 'In Stock' END",
"CASE WHEN 1 > 0 THEN 1 ELSE 0 END",
"CASE WHEN is_active THEN 'active' WHEN is_expired THEN 'expired' ELSE 'active' END",
"'text' LIKE 'pattern'",
"'text' NOT LIKE 'pattern'",
}

for i, e := range exps {
Expand All @@ -2056,3 +2058,46 @@ func TestExprString(t *testing.T) {
})
}
}

func TestLogicOperatorPrecedence(t *testing.T) {
type testCase struct {
input string
expected string
}

testCases := []testCase{
// simple precedence
{input: "NOT true", expected: "(NOT true)"},
{input: "true AND false OR true", expected: "((true AND false) OR true)"},
{input: "NOT true AND false", expected: "((NOT true) AND false)"},
{input: "NOT true OR false", expected: "((NOT true) OR false)"},

// parentheses override precedence
{input: "(true OR false) AND true", expected: "((true OR false) AND true)"},

// multiple NOTs
{input: "NOT NOT true AND false", expected: "((NOT (NOT true)) AND false)"},

// complex nesting
{input: "true AND (false OR (NOT false))", expected: "(true AND (false OR (NOT false)))"},
{input: "NOT (true AND false) OR true", expected: "((NOT (true AND false)) OR true)"},

// AND/OR with nested groups
{input: "(true AND false) AND (true OR false)", expected: "((true AND false) AND (true OR false))"},
{input: "(true OR false) OR (NOT (true AND false))", expected: "((true OR false) OR (NOT (true AND false)))"},

// deep nesting
{input: "(true AND (false OR (NOT true))) OR (NOT false)", expected: "((true AND (false OR (NOT true))) OR (NOT false))"},

// chain of operators
{input: "true AND false OR true AND NOT false OR true", expected: "(((true AND false) OR (true AND (NOT false))) OR true)"},
}

for _, tc := range testCases {
t.Run(tc.input, func(t *testing.T) {
e, err := ParseExpFromString(tc.input)
require.NoError(t, err)
require.Equal(t, tc.expected, e.String())
})
}
}
19 changes: 14 additions & 5 deletions embedded/sql/sql_grammar.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func setResult(l yyLexer, stmts []SQLStmt) {
%token <id> NPARAM
%token <pparam> PPARAM
%token <joinType> JOINTYPE
%token <logicOp> LOP
%token <logicOp> AND OR
%token <cmpOp> CMPOP
%token <id> IDENTIFIER
%token <sqlType> TYPE
Expand All @@ -102,10 +102,14 @@ func setResult(l yyLexer, stmts []SQLStmt) {

%left ','
%right AS
%left LOP

%left OR
%left AND

%right LIKE
%right NOT
%left CMPOP

%left CMPOP
%left '+' '-'
%left '*' '/' '%'
%left '.'
Expand Down Expand Up @@ -1216,9 +1220,14 @@ binExp:
$$ = &NumExp{left: $1, op: MODOP, right: $3}
}
|
exp LOP exp
exp AND exp
{
$$ = &BinBoolExp{left: $1, op: And, right: $3}
}
|
exp OR exp
{
$$ = &BinBoolExp{left: $1, op: $2, right: $3}
$$ = &BinBoolExp{left: $1, op: Or, right: $3}
}
|
exp CMPOP exp
Expand Down
Loading

0 comments on commit b7ff0e6

Please sign in to comment.