Extra expression parsing and formatting tweaks

* persist `raw` on AST nodes
* remove unneeded groupping by the ql pkg
* allow store flavours to specify what operations need to be enclosed
* extra tests

pkg/qlng/ast_nodes.go

@@ -265,10 +265,14 @@ func (nn parserNodes) ToAST() (out *ASTNode) {
 		auxArgs = aux
 	}
 
-	return &ASTNode{
-		Ref:  "group",
-		Args: auxArgs,
+	if len(auxArgs) > 1 {
+		return &ASTNode{
+			Ref:  "group",
+			Args: auxArgs,
+		}
 	}
+
+	return auxArgs[0]
 }
 
 // A simplified type registry for the types that QL needs to understand

pkg/report/frame.go

@@ -1,6 +1,7 @@
 package report
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"strings"
@@ -108,28 +109,6 @@ func MakeColumnOfKind(k string) *FrameColumn {
 	}
 }
 
-func KindOf(v expr.TypedValue) string {
-	// @todo ...
-	if v == nil {
-		return "String"
-	}
-
-	switch v.Type() {
-	case "Integer",
-		"UnsignedInteger",
-		"Float":
-		return "Number"
-	case "DateTime":
-		return "DateTime"
-	case "ID":
-		return "Ref"
-	case "Boolean":
-		return "Checkbox"
-	default:
-		return "String"
-	}
-}
-
 func (f *Filter) UnmarshalJSON(data []byte) (err error) {
 	var aux interface{}
 	if err = json.Unmarshal(data, &aux); err != nil {
@@ -152,6 +131,11 @@ func (f *Filter) UnmarshalJSON(data []byte) (err error) {
 		return
 	}
 
+	// special case for empty JSON
+	if bytes.Equal([]byte{'{', '}'}, data) {
+		return
+	}
+
 	// non-string is considered an AST and we parse that
 	if err = json.Unmarshal(data, &f.ASTNode); err != nil {
 		return
@@ -169,6 +153,7 @@ func (f *Filter) UnmarshalJSON(data []byte) (err error) {
 		}
 
 		aux, err := p.Parse(n.Raw)
+		aux.Raw = n.Raw
 		if err != nil {
 			return false, n, err
 		}

pkg/report/step_group.go

@@ -1,6 +1,7 @@
 package report
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -120,6 +121,11 @@ func (def *colDef) UnmarshalJSON(data []byte) (err error) {
 		return
 	}
 
+	// special case for empty JSON
+	if bytes.Equal([]byte{'{', '}'}, data) {
+		return
+	}
+
 	// non-string is considered an AST and we parse that
 	if err = json.Unmarshal(data, &def.ASTNode); err != nil {
 		return
@@ -137,6 +143,7 @@ func (def *colDef) UnmarshalJSON(data []byte) (err error) {
 		}
 
 		aux, err := p.Parse(n.Raw)
+		aux.Raw = n.Raw
 		if err != nil {
 			return false, n, err
 		}

store/rdbms/ast_transformer.go

@@ -20,8 +20,8 @@ type (
 
 		ResultType string
 	}
-	ASTFormatterFn func(n *qlng.ASTNode, aa ...FormattedASTArgs) (bool, string, []interface{}, error)
-	HandlerSig     func(aa ...FormattedASTArgs) (string, []interface{}, error)
+	ASTFormatterFn func(n *qlng.ASTNode) HandlerSig
+	HandlerSig     func(aa ...FormattedASTArgs) (string, []interface{}, bool, error)
 
 	exprHandler struct {
 		Args   argSet
@@ -185,8 +185,8 @@ var (
 		// generic stuff
 		"null": {
 			Result: wrapRes("Null"),
-			Handler: func(aa ...FormattedASTArgs) (string, []interface{}, error) {
-				return "NULL", nil, nil
+			Handler: func(aa ...FormattedASTArgs) (string, []interface{}, bool, error) {
+				return "NULL", nil, true, nil
 			},
 		},
 
@@ -218,10 +218,10 @@ func (t *astTransformer) SetPlaceholder(use bool) {
 
 // ToSql conforms the struct to squirrel allowing trivial RDBMS use
 func (t *astTransformer) ToSql() (string, []interface{}, error) {
-	return t.toSql(t.root)
+	return t.toSql(t.root, true)
 }
 
-func (t *astTransformer) toSql(n *qlng.ASTNode) (string, []interface{}, error) {
+func (t *astTransformer) toSql(n *qlng.ASTNode, isRoot bool) (string, []interface{}, error) {
 	// Leaf edge-cases
 	switch {
 	case n.Symbol != "":
@@ -233,7 +233,7 @@ func (t *astTransformer) toSql(n *qlng.ASTNode) (string, []interface{}, error) {
 	// Process arguments for the op.
 	args := make([]FormattedASTArgs, len(n.Args))
 	for i, a := range n.Args {
-		s, pp, err := t.toSql(a)
+		s, pp, err := t.toSql(a, false)
 		if err != nil {
 			return "", nil, err
 		}
@@ -250,21 +250,29 @@ func (t *astTransformer) toSql(n *qlng.ASTNode) (string, []interface{}, error) {
 			continue
 		}
 
-		if ok, s, args, err := c(n, args...); ok {
+		if h := c(n); h != nil {
+			s, args, encl, err := h(args...)
+			if !isRoot && !encl && len(args) > 1 {
+				s = "(" + s + ")"
+			}
 			return s, args, err
 		}
 	}
 
 	if n.Ref == "group" {
-		return bracketHandler(args...)
+		s, args, _, err := bracketHandler(args...)
+		return s, args, err
 	}
 
 	// Default handlers
 	if e, ok := sqlExprRegistry[n.Ref]; !ok {
 		return "", nil, fmt.Errorf("unknown expression: handler not defined: %s", n.Ref)
 	} else {
-		s, args, err := e.Handler(args...)
-		return s, args, err
+		s, _args, encl, err := e.Handler(args...)
+		if !isRoot && !encl && len(args) > 1 {
+			s = "(" + s + ")"
+		}
+		return s, _args, err
 	}
 }
 

store/rdbms/ast_transformer_handlers.go

@@ -48,7 +48,12 @@ func (t *astTransformer) handleValue(n *qlng.ASTNode) (string, []interface{}, er
 }
 
 func makeGenericBoolHandler(op string) HandlerSig {
-	return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) {
+	return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
+		if len(aa) < 1 {
+			err = fmt.Errorf("expecting 1 or more arguments, got %d", len(aa))
+			return
+		}
+
 		outPts := make([]string, len(aa))
 		args = make([]interface{}, 0, 10)
 		for i, a := range aa {
@@ -62,7 +67,9 @@ func makeGenericBoolHandler(op string) HandlerSig {
 }
 
 func makeGenericBracketHandler(bb ...string) HandlerSig {
-	return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) {
+	return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
+		selfEnclosed = true
+
 		if len(aa) != 1 {
 			err = fmt.Errorf("expecting 1 argument, got %d", len(aa))
 			return
@@ -75,7 +82,9 @@ func makeGenericBracketHandler(bb ...string) HandlerSig {
 }
 
 func makeGenericCompHandler(comp string) HandlerSig {
-	return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) {
+	return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
+		selfEnclosed = true
+
 		if len(aa) != 2 {
 			err = fmt.Errorf("expecting 2 arguments, got %d", len(aa))
 			return
@@ -88,7 +97,9 @@ func makeGenericCompHandler(comp string) HandlerSig {
 }
 
 func makeGenericAggFncHandler(fnc string) HandlerSig {
-	return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) {
+	return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
+		selfEnclosed = true
+
 		if fnc == "COUNT" && len(aa) == 0 {
 			out = "COUNT(*)"
 			return
@@ -106,7 +117,9 @@ func makeGenericAggFncHandler(fnc string) HandlerSig {
 }
 
 func makeGenericTypecastHandler(t string) HandlerSig {
-	return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) {
+	return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
+		selfEnclosed = true
+
 		if len(aa) != 1 {
 			err = fmt.Errorf("expecting 1 argument, got %d", len(aa))
 			return
@@ -119,9 +132,11 @@ func makeGenericTypecastHandler(t string) HandlerSig {
 }
 
 func makeGenericFilterFncHandler(fnc string) HandlerSig {
-	return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) {
+	return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
+		selfEnclosed = true
+
 		if len(aa) == 0 {
-			return fmt.Sprintf("%s()", fnc), nil, nil
+			return fmt.Sprintf("%s()", fnc), nil, selfEnclosed, nil
 		}
 
 		args = make([]interface{}, 0, len(aa))

store/rdbms/compose_record_datasource.go

@@ -196,7 +196,7 @@ func (r *recordDatasource) Group(d report.GroupDefinition, name string) (bool, e
 		q = q.Column(squirrel.Alias(tr, c.Name))
 	}
 
-	if d.Filter != nil {
+	if d.Filter != nil && d.Filter.ASTNode != nil {
 		q = q.Having(r.store.ASTTransformer(d.Filter.ASTNode))
 	}
 
@@ -260,12 +260,12 @@ func (r *recordDatasource) preloadQuery(def *report.FrameDefinition) (squirrel.S
 
 	// when filtering/sorting, wrap the base query in a sub-select, so we don't need to
 	// worry about exact column names.
-	if def.Filter != nil || def.Sort != nil {
+	if def.Filter != nil && def.Filter.ASTNode != nil || def.Sort != nil {
 		q = squirrel.Select("*").FromSelect(q, "w_base")
 	}
 
 	// - filtering
-	if def.Filter != nil {
+	if def.Filter != nil && def.Filter.ASTNode != nil {
 		q = q.Where(r.store.ASTTransformer(def.Filter.ASTNode))
 	}
 
@@ -430,7 +430,9 @@ func (r *recordDatasource) baseQuery(f *report.Filter) (sqb squirrel.SelectBuild
 			return
 		}
 
-		sqb = sqb.Where(r.store.ASTTransformer(f.ASTNode))
+		if f != nil && f.ASTNode != nil {
+			sqb = sqb.Where(r.store.ASTTransformer(f.ASTNode))
+		}
 	}
 
 	return sqb, nil

store/sqlite3/sql_functions.go

@@ -13,7 +13,7 @@ var (
 	sqlExprRegistry = map[string]rdbms.HandlerSig{
 		// functions
 		// - filtering
-		"now": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) {
+		"now": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
 			if len(aa) != 0 {
 				err = fmt.Errorf("expecting 0 arguments, got %d", len(aa))
 				return
@@ -22,7 +22,7 @@ var (
 			out = "DATE('now')"
 			return
 		},
-		"quarter": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) {
+		"quarter": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
 			if len(aa) != 1 {
 				err = fmt.Errorf("expecting 1 arguments, got %d", len(aa))
 				return
@@ -32,7 +32,7 @@ var (
 			args = aa[0].Args
 			return
 		},
-		"year": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) {
+		"year": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
 			if len(aa) != 1 {
 				err = fmt.Errorf("expecting 1 arguments, got %d", len(aa))
 				return
@@ -42,7 +42,7 @@ var (
 			args = aa[0].Args
 			return
 		},
-		"date": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) {
+		"date": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) {
 			if len(aa) != 1 {
 				err = fmt.Errorf("expecting 1 arguments, got %d", len(aa))
 				return
@@ -55,14 +55,8 @@ var (
 	}
 )
 
-func sqlASTFormatter(n *qlng.ASTNode, aa ...rdbms.FormattedASTArgs) (ok bool, out string, args []interface{}, err error) {
-	e, ok := sqlExprRegistry[n.Ref]
-	if !ok {
-		return
-	}
-
-	out, args, err = e(aa...)
-	return
+func sqlASTFormatter(n *qlng.ASTNode) rdbms.HandlerSig {
+	return sqlExprRegistry[n.Ref]
 }
 
 func sqlFunctionHandler(f ql.Function) (ql.ASTNode, error) {

tests/reporter/9007_filtering_specials_test.go

@@ -11,7 +11,6 @@ func Test9007_filtering_specials(t *testing.T) {
 		ff        = loadNoErrMulti(ctx, h, m, dd...)
 	)
 
-	h.a.Len(ff, 3)
 	f := ff[0]
 	h.a.Equal(12, f.Size())
 	h.a.Equal("empty_filter", f.Name)
@@ -26,4 +25,19 @@ func Test9007_filtering_specials(t *testing.T) {
 	h.a.Equal(12, f.Size())
 	h.a.Equal("true_filter", f.Name)
 	h.a.Equal("first_name<String>, last_name<String>", f.Columns.String())
+
+	f = ff[3]
+	h.a.Equal(4, f.Size())
+	h.a.Equal("weird_filter", f.Name)
+	h.a.Equal("first_name<String>, last_name<String>", f.Columns.String())
+	checkRows(h, f,
+		"Maria, Königsmann",
+		"Engel, Loritz",
+		"Maria, Krüger",
+		"Engel, Kiefer")
+
+	f = ff[4]
+	h.a.Equal(12, f.Size())
+	h.a.Equal("empty_filter_object", f.Name)
+	h.a.Equal("first_name<String>, last_name<String>", f.Columns.String())
 }

tests/reporter/9008_filtering_eval_order_test.go

@@ -0,0 +1,34 @@
+package reporter
+
+import (
+	"testing"
+)
+
+func Test9008_filtering_eval_order(t *testing.T) {
+	var (
+		ctx, h, s = setup(t)
+		m, _, dd  = loadScenarioOwnDM(ctx, s, t, h)
+		ff        = loadNoErrMulti(ctx, h, m, dd...)
+	)
+
+	f := ff[0]
+	h.a.Equal(4, f.Size())
+	h.a.Equal("c1", f.Name)
+	h.a.Equal("first_name<String>, numbers<Number>", f.Columns.String())
+	checkRows(h, f,
+		"test1, 11",
+		"test1, 11",
+		"test2, 11",
+		"test1, 14")
+
+	f = ff[1]
+	h.a.Equal(4, f.Size())
+	h.a.Equal("c2", f.Name)
+	h.a.Equal("first_name<String>, numbers<Number>", f.Columns.String())
+	checkRows(h, f,
+		"test1, 11",
+		"test1, 11",
+		"test2, 11",
+		"test1, 14")
+
+}

tests/reporter/9009_filtering_vaidation_test.go

@@ -0,0 +1,14 @@
+package reporter
+
+import (
+	"testing"
+)
+
+func Test9009_filtering_validation(t *testing.T) {
+	var (
+		ctx, h, s = setup(t)
+		m, _, dd  = loadScenario(ctx, s, t, h)
+	)
+
+	loadErr(ctx, h, m, dd[0], "could not build query: expecting 1 or more arguments, got 0")
+}

tests/reporter/main_test.go

@@ -182,6 +182,12 @@ func loadNoErr(ctx context.Context, h helper, m report.M, dd ...*report.FrameDef
 	return
 }
 
+func loadErr(ctx context.Context, h helper, m report.M, d *report.FrameDefinition, msg string) {
+	_, err := m.Load(ctx, d)
+	h.a.Error(err)
+	h.a.Contains(err.Error(), msg)
+}
+
 // loadNoErrMulti is a little wrapper that does some preprocessing on the frame definitions.
 // It is a copy from the system/service/report.
 //
@@ -255,6 +261,25 @@ func loadScenarioWithName(ctx context.Context, s store.Storer, t *testing.T, h h
 	return m, rr, rr.Frames
 }
 
+func loadScenarioOwnDM(ctx context.Context, s store.Storer, t *testing.T, h helper) (report.M, *auxReport, report.FrameDefinitionSet) {
+	return loadScenarioOwnDMWithName(ctx, s, t, h, "S"+t.Name()[4:])
+}
+
+func loadScenarioOwnDMWithName(ctx context.Context, s store.Storer, t *testing.T, h helper, scenario string) (report.M, *auxReport, report.FrameDefinitionSet) {
+	var (
+		providers = map[string]report.DatasourceProvider{
+			"composeRecords": service.DefaultRecord,
+		}
+	)
+
+	cleanup(ctx, h, s)
+	parseEnvoy(ctx, s, h, path.Join("testdata", scenario, "data_model"))
+	rr := parseReport(h, path.Join("testdata", scenario, "report.json"))
+	m := modelReport(ctx, h, providers, rr)
+
+	return m, rr, rr.Frames
+}
+
 func cleanup(ctx context.Context, h helper, s store.Storer) {
 	h.noError(s.TruncateComposeNamespaces(ctx))
 	h.noError(s.TruncateComposeModules(ctx))

tests/reporter/testdata/S9007_filtering_specials/report.json

@@ -7,7 +7,9 @@
       "definition": {
         "module": "user",
         "namespace": "ns"
-      }
+      },
+
+      "filter": {}
   }}}],
   "frames": [{
     "name":   "empty_filter",
@@ -36,5 +38,35 @@
     ],
 
     "filter": "true"
+  }, {
+    "name":   "weird_filter",
+    "source": "users",
+    "columns": [
+      { "name": "first_name", "label": "first_name" },
+      { "name": "last_name", "label": "last_name" }
+    ],
+
+    "filter": {
+      "ref": "and",
+      "args": [
+        {
+          "ref": "or",
+          "args": [
+            { "raw": "first_name = 'Maria'" },
+            { "raw": "first_name = 'Engel'" }
+          ]
+        },
+        { "ref": "or", "args": [{ "raw": "number_of_numbers > 40" }] }
+      ]
+    }
+  }, {
+    "name":   "empty_filter_object",
+    "source": "users",
+    "columns": [
+      { "name": "first_name", "label": "first_name" },
+      { "name": "last_name", "label": "last_name" }
+    ],
+
+    "filter": {}
   }]
 }

tests/reporter/testdata/S9008_filtering_eval_order/data_model/data_users.csv

@@ -0,0 +1,10 @@
+id,first_name,numbers
+1,test1,1
+2,test1,11
+3,test2,1
+4,no,100
+5,test1,11
+6,test1,2
+7,test2,11
+8,test1,14
+9,no,100

tests/reporter/testdata/S9008_filtering_eval_order/data_model/datamodel.yaml

@@ -0,0 +1,23 @@
+namespaces:
+  ns:
+    name: ns name
+
+modules:
+  user:
+    records:
+      source: data_users.csv
+      key: id
+      mapping:
+        id: /
+        first_name:
+          field: first_name
+        numbers:
+          field: numbers
+
+    fields:
+      first_name:
+        label: first_name label
+        kind: String
+      numbers:
+        label: numbers label
+        kind: Number

tests/reporter/testdata/S9008_filtering_eval_order/report.json

@@ -0,0 +1,82 @@
+{
+  "handle": "testing_report",
+  "sources": [{
+    "step": { "load": {
+      "name": "users",
+      "source": "composeRecords",
+      "definition": {
+        "module": "user",
+        "namespace": "ns"
+      }
+  }}}],
+  "frames": [{
+    "name":   "c1",
+    "source": "users",
+    "columns": [
+      { "name": "first_name", "label": "first_name" },
+      { "name": "numbers", "label": "numbers" }
+    ],
+
+    "filter": {
+      "ref": "and",
+      "args": [
+        {
+          "ref": "or",
+          "args": [
+            { "raw": "first_name = 'test1'" },
+            { "raw": "first_name = 'test2'" }
+          ]
+        },
+        { "ref": "or", "args": [{ "raw": "numbers > 10" }] }
+      ]
+    }
+  }, {
+    "name":   "c2",
+    "source": "users",
+    "columns": [
+      { "name": "first_name", "label": "first_name" },
+      { "name": "numbers", "label": "numbers" }
+    ],
+
+    "filter": {
+      "ref": "and",
+      "args": [
+        {
+          "ref": "or",
+          "args": [{
+            "ref": "eq",
+            "args": [{
+              "symbol": "first_name"
+            }, {
+              "value": {
+                "@type": "String",
+                "@value": "test1"
+              }
+            }]
+          }, {
+            "ref": "eq",
+            "args": [{
+              "symbol": "first_name"
+            }, {
+              "value": {
+                "@type": "String",
+                "@value": "test2"
+              }
+            }]
+          }]
+        },
+        { "ref": "or", "args": [{
+          "ref": "gt",
+          "args": [{
+            "symbol": "numbers"
+          }, {
+            "value": {
+              "@type": "Integer",
+              "@value": 10
+            }
+          }]
+        }] }
+      ]
+    }
+  }]
+}

tests/reporter/testdata/S9009_filtering_validation/report.json

@@ -0,0 +1,25 @@
+{
+  "handle": "testing_report",
+  "sources": [{
+    "step": { "load": {
+      "name": "users",
+      "source": "composeRecords",
+      "definition": {
+        "module": "user",
+        "namespace": "ns"
+      }
+  }}}],
+  "frames": [{
+    "name":   "empty_conjunction",
+    "source": "users",
+    "columns": [
+      { "name": "first_name", "label": "first_name" },
+      { "name": "last_name", "label": "last_name" }
+    ],
+
+    "filter": {
+      "ref": "and",
+      "args": []
+    }
+  }]
+}