From be1420f1bc3c1fe9b0a8efd0bbbf69b77b19337c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Toma=C5=BE=20Jerman?= Date: Fri, 29 Nov 2019 13:00:55 +0100 Subject: [PATCH 1/3] Fix QL parsing for nested expressions & formatting Parsing: ((A) AND (B)) got parsed into [ASTNodes{A}] istead of [ASTNodes{A}, ASTNode{AND}, ASTNodes{B}]. Formatting into SQL: [ASTNodes{A}, ASTNode{AND}, ASTNodes{B}] got formatted into A AND B instead of (A) AND (B). --- pkg/ql/ast_parser.go | 8 ++++++++ pkg/ql/squirrel.go | 8 +++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/ql/ast_parser.go b/pkg/ql/ast_parser.go index 5c1e8f244..514c6610b 100644 --- a/pkg/ql/ast_parser.go +++ b/pkg/ql/ast_parser.go @@ -194,11 +194,19 @@ checkToken: goto next case PARENTHESIS_OPEN: p.level++ + dpth := p.level if sub, err := p.parseExpr(p.nextToken()); err != nil { return nil, err } else { list = append(list, sub) } + // Allow parent level to continue parsing. + // Example: ((A) AND (B)) + // +1 since PARENTHESIS_CLOSE decrease level + if p.level > 0 && (p.level+1) != dpth { + p.nextToken() + goto next + } default: return nil, fmt.Errorf("unexpected token while parsing expression (%v)", t) } diff --git a/pkg/ql/squirrel.go b/pkg/ql/squirrel.go index 2128b21ce..c613cc91f 100644 --- a/pkg/ql/squirrel.go +++ b/pkg/ql/squirrel.go @@ -18,7 +18,13 @@ func (nn ASTNodes) ToSql() (out string, args []interface{}, err error) { if _out, _args, err = s.ToSql(); err != nil { return } else { - out = out + _out + if _, ok := s.(ASTNodes); ok { + // Nested nodes should be wrapped + // Example: ((A) AND (B)) + out = out + "(" + _out + ")" + } else { + out = out + _out + } args = append(args, _args...) } From 63e3cff8ab1f97dbd55db461c6fd782e9ac38ccd Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Mon, 2 Dec 2019 10:46:27 +0100 Subject: [PATCH 2/3] Add tests, improve parsing of nested expressions --- pkg/ql/ast_parser.go | 5 +++-- pkg/ql/ast_parser_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/pkg/ql/ast_parser.go b/pkg/ql/ast_parser.go index 514c6610b..b6e37b6bc 100644 --- a/pkg/ql/ast_parser.go +++ b/pkg/ql/ast_parser.go @@ -193,17 +193,18 @@ checkToken: list = append(list, String{Value: t.literal}) goto next case PARENTHESIS_OPEN: + depth := p.level p.level++ - dpth := p.level if sub, err := p.parseExpr(p.nextToken()); err != nil { return nil, err } else { list = append(list, sub) } + // Allow parent level to continue parsing. // Example: ((A) AND (B)) // +1 since PARENTHESIS_CLOSE decrease level - if p.level > 0 && (p.level+1) != dpth { + if (p.level + 1) != depth { p.nextToken() goto next } diff --git a/pkg/ql/ast_parser_test.go b/pkg/ql/ast_parser_test.go index 2b2e0188f..85a5ac283 100644 --- a/pkg/ql/ast_parser_test.go +++ b/pkg/ql/ast_parser_test.go @@ -183,6 +183,39 @@ func TestAstParser_Parser(t *testing.T) { Null{}, }, }, + { + parser: NewParser().ParseExpression, + in: `((foo1) AND (foo2))`, + tree: ASTNodes{ + ASTNodes{Ident{Value: "foo1"}}, + Operator{"AND"}, + ASTNodes{Ident{Value: "foo2"}}, + }, + }, + { + parser: NewParser().ParseExpression, + in: `((foo1) AND (foo2) AND foo3)`, + tree: ASTNodes{ + ASTNodes{Ident{Value: "foo1"}}, + Operator{"AND"}, + ASTNodes{Ident{Value: "foo2"}}, + Operator{"AND"}, + Ident{Value: "foo3"}, + }, + }, + { + parser: NewParser().ParseExpression, + in: `((foo1) AND (foo2)) AND foo3`, + tree: ASTNodes{ + ASTNodes{ + ASTNodes{Ident{Value: "foo1"}}, + Operator{"AND"}, + ASTNodes{Ident{Value: "foo2"}}, + }, + Operator{"AND"}, + Ident{Value: "foo3"}, + }, + }, } for i, test := range tests { From 3452db343d9890a0e1489c02c7e12d784a5babf7 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Mon, 2 Dec 2019 11:27:43 +0100 Subject: [PATCH 3/3] Modify record organizing test (effects of QL parsing changes) --- tests/compose/record_exec_test.go | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/tests/compose/record_exec_test.go b/tests/compose/record_exec_test.go index 77898e3ed..a774d5c21 100644 --- a/tests/compose/record_exec_test.go +++ b/tests/compose/record_exec_test.go @@ -57,7 +57,7 @@ func TestRecordExec(t *testing.T) { ) } - assertSort := func(expectedHandles string) { + assertSort := func(expectedHandles, expectedCats string) { // Using record service for fetching to avoid value pre-fetching etc.. set, _, err := service.DefaultRecord.With(h.secCtx()).Find(types.RecordFilter{ ModuleID: module.ID, @@ -70,6 +70,8 @@ func TestRecordExec(t *testing.T) { h.a.NotNil(set) actualHandles := "" + actualCats := "" + _ = set.Walk(func(r *types.Record) error { v := r.Values.FilterByName("handle") @@ -79,10 +81,13 @@ func TestRecordExec(t *testing.T) { actualHandles += strconv.Itoa(len(v)) } + actualCats += r.Values.FilterByName("category")[0].Value[3:] + return nil }) h.a.Equal(expectedHandles, actualHandles) + h.a.Equal(expectedCats, actualCats) } var ( @@ -110,8 +115,9 @@ func TestRecordExec(t *testing.T) { "i": strconv.FormatUint(iRec.ID, 10), } - assertSort("abcdefghi") + assertSort("abcdefghi", "111222333") + // Move a to the middle h.apiSendRecordExec(module.NamespaceID, module.ID, "organize", request.ProcedureArgs{ {"recordID", rr["a"]}, {"positionField", "position"}, @@ -120,8 +126,9 @@ func TestRecordExec(t *testing.T) { Assert(helpers.AssertNoErrors). End() - assertSort("bcdeafghi") + assertSort("bcdeafghi", "112212333") + // Move i to the beginning h.apiSendRecordExec(module.NamespaceID, module.ID, "organize", request.ProcedureArgs{ {"recordID", rr["i"]}, {"positionField", "position"}, @@ -130,8 +137,11 @@ func TestRecordExec(t *testing.T) { Assert(helpers.AssertNoErrors). End() - assertSort("ibcdeafgh") + // bcdeafghi + // v<------^ + assertSort("ibcdeafgh", "311221233") + // Move b to the 5th place h.apiSendRecordExec(module.NamespaceID, module.ID, "organize", request.ProcedureArgs{ {"recordID", rr["b"]}, {"filter", "category = 'CAT1'"}, @@ -141,8 +151,11 @@ func TestRecordExec(t *testing.T) { Assert(helpers.AssertNoErrors). End() - assertSort("idecbfgah") + // ibcdeafgh + // ^->v + assertSort("icdebfagh", "312212133") + // This will keep order of letters but move b to category-2 h.apiSendRecordExec(module.NamespaceID, module.ID, "organize", request.ProcedureArgs{ {"recordID", rr["b"]}, {"groupField", "category"}, @@ -152,7 +165,10 @@ func TestRecordExec(t *testing.T) { Assert(helpers.AssertNoErrors). End() - assertSort("idecbfgah") + // icdebfagh + // ^ + assertSort("icdebfagh", "312222133") + rsv, err := h.repoRecord().LoadValues([]string{"category"}, []uint64{bRec.ID}) h.a.NoError(err) h.a.NotNil(rsv)