From 11c225c6f4ac044bdf4806d6a5f61fc32afe804d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Toma=C5=BE=20Jerman?= Date: Fri, 24 Sep 2021 12:49:30 +0200 Subject: [PATCH] Post testing reporter tweaks * add support for existence checking (IS (NOT) NULL) * cover edgecase where either join dimension is empty * add support for negation operator --- compose/service/record_datasource.go | 6 +-- pkg/qlng/ast_nodes.go | 32 +++++++++++----- pkg/qlng/parser_nodes.go | 9 ++++- pkg/report/step_join.go | 37 ++++++++++++------- store/rdbms/ast_transformer.go | 24 +++++++++++- store/rdbms/ast_transformer_handlers.go | 15 ++++++++ tests/reporter/filter_existence_test.go | 35 ++++++++++++++++++ tests/reporter/filter_negation_test.go | 32 ++++++++++++++++ tests/reporter/filter_temporal_test.go | 35 ++++++++++++++++++ tests/reporter/join_empty_foreign_test.go | 18 +++++++++ tests/reporter/model_describe_basic_test.go | 8 ++-- .../testdata/filter_existence/report.json | 22 +++++++++++ .../testdata/filter_negation/report.json | 22 +++++++++++ .../testdata/filter_temporal/report.json | 23 ++++++++++++ .../testdata/join_empty_foreign/report.json | 37 +++++++++++++++++++ 15 files changed, 322 insertions(+), 33 deletions(-) create mode 100644 tests/reporter/filter_existence_test.go create mode 100644 tests/reporter/filter_negation_test.go create mode 100644 tests/reporter/filter_temporal_test.go create mode 100644 tests/reporter/join_empty_foreign_test.go create mode 100644 tests/reporter/testdata/filter_existence/report.json create mode 100644 tests/reporter/testdata/filter_negation/report.json create mode 100644 tests/reporter/testdata/filter_temporal/report.json create mode 100644 tests/reporter/testdata/join_empty_foreign/report.json diff --git a/compose/service/record_datasource.go b/compose/service/record_datasource.go index c1416ad19..072694d2c 100644 --- a/compose/service/record_datasource.go +++ b/compose/service/record_datasource.go @@ -92,7 +92,7 @@ func (svc record) Datasource(ctx context.Context, ld *report.LoadStepDefinition) } // Sys fields - c = report.MakeColumnOfKind("Date") + c = report.MakeColumnOfKind("DateTime") c.System = true c.Name = "createdAt" c.Label = "Created at" @@ -104,7 +104,7 @@ func (svc record) Datasource(ctx context.Context, ld *report.LoadStepDefinition) c.Label = "Created by" cols = append(cols, c) - c = report.MakeColumnOfKind("Date") + c = report.MakeColumnOfKind("DateTime") c.System = true c.Name = "updatedAt" c.Label = "Updated at" @@ -116,7 +116,7 @@ func (svc record) Datasource(ctx context.Context, ld *report.LoadStepDefinition) c.Label = "Updated by" cols = append(cols, c) - c = report.MakeColumnOfKind("Date") + c = report.MakeColumnOfKind("DateTime") c.System = true c.Name = "deletedAt" c.Label = "Deleted at" diff --git a/pkg/qlng/ast_nodes.go b/pkg/qlng/ast_nodes.go index d86d7ce01..21877cc59 100644 --- a/pkg/qlng/ast_nodes.go +++ b/pkg/qlng/ast_nodes.go @@ -279,18 +279,30 @@ func (nn parserNodes) ToAST() (out *ASTNode) { } // Have the op consume what it needs. - // Currently we only have binary operators (the !unary ones) so this is fine. arg := auxArgs[bestOpIx] - arg.Args = append(arg.Args, auxArgs[bestOpIx-1], auxArgs[bestOpIx+1]) - // this is not needed anymore so we can remove it - arg.pMeta = nil + if !isUnary(arg.Ref) { + arg.Args = append(arg.Args, auxArgs[bestOpIx-1], auxArgs[bestOpIx+1]) + // this is not needed anymore so we can remove it + arg.pMeta = nil - // Remove the consumed bits and replace it with the new bit - aux := auxArgs[0 : bestOpIx-1] - aux = append(aux, arg) - // +1 for right side, +1 because the left index is inclusive - aux = append(aux, auxArgs[bestOpIx+2:]...) - auxArgs = aux + // Remove the consumed bits and replace it with the new bit + aux := auxArgs[0 : bestOpIx-1] + aux = append(aux, arg) + // +1 for right side, +1 because the left index is inclusive + aux = append(aux, auxArgs[bestOpIx+2:]...) + auxArgs = aux + } else { + arg.Args = append(arg.Args, auxArgs[bestOpIx+1]) + // this is not needed anymore so we can remove it + arg.pMeta = nil + + // Remove the consumed bits and replace it with the new bit + aux := auxArgs[0:bestOpIx] + aux = append(aux, arg) + // +1 because the left index is inclusive + aux = append(aux, auxArgs[bestOpIx+2:]...) + auxArgs = aux + } } if len(auxArgs) > 1 { diff --git a/pkg/qlng/parser_nodes.go b/pkg/qlng/parser_nodes.go index 1eb95c5fc..6850994e8 100644 --- a/pkg/qlng/parser_nodes.go +++ b/pkg/qlng/parser_nodes.go @@ -87,12 +87,19 @@ var ( `*`: {name: `mult`, weight: 10}, `/`: {name: `div`, weight: 10}, + // modifiers + `!`: {name: `not`, weight: 0}, + // str comp. `LIKE`: {name: `like`, weight: 40}, `NOT LIKE`: {name: `nlike`, weight: 40}, } ) +func isUnary(s string) bool { + return s == "!" || s == "not" +} + func getOp(op string) *opDef { o, ok := ops[strings.ToUpper(op)] if !ok { @@ -149,7 +156,7 @@ func (nn parserNodes) Validate() (err error) { return fmt.Errorf("empty set") } - if op, ok := nn[0].(operator); ok { + if op, ok := nn[0].(operator); ok && !isUnary(op.kind) { return fmt.Errorf("malformed expression, unexpected operator '%s' at first node", op) } diff --git a/pkg/report/step_join.go b/pkg/report/step_join.go index 64247f214..47fd090f6 100644 --- a/pkg/report/step_join.go +++ b/pkg/report/step_join.go @@ -695,30 +695,39 @@ func (d *joinedDataset) foreignBasedLoad(ctx context.Context, cap int, local, fo // updateBuffers updates existing frame buffers with newly provided ones func (d *joinedDataset) updateBuffers(local, foreign []*Frame) { // - local - if len(d.localFrames) == 0 { - d.localFrames = local - } else { - if d.partitioned { - d.localFrames = append(d.localFrames, local...) + if len(local) != 0 { + if len(d.localFrames) == 0 { + d.localFrames = local } else { - d.localFrames[0].Rows = append(d.localFrames[0].Rows, local[0].Rows...) + if d.partitioned { + d.localFrames = append(d.localFrames, local...) + } else { + d.localFrames[0].Rows = append(d.localFrames[0].Rows, local[0].Rows...) + } } } // - foreign - if d.foreignSourceIndex == nil { - d.foreignSourceIndex = make(map[string]int) - } - if ix, ok := d.foreignSourceIndex[foreign[0].Ref]; !ok { - d.foreignSourceIndex[foreign[0].Ref] = len(d.foreignFrames) - d.foreignFrames = append(d.foreignFrames, foreign) - } else { - d.foreignFrames[ix] = append(d.foreignFrames[ix], foreign...) + if len(foreign) != 0 { + if d.foreignSourceIndex == nil { + d.foreignSourceIndex = make(map[string]int) + } + if ix, ok := d.foreignSourceIndex[foreign[0].Ref]; !ok { + d.foreignSourceIndex[foreign[0].Ref] = len(d.foreignFrames) + d.foreignFrames = append(d.foreignFrames, foreign) + } else { + d.foreignFrames[ix] = append(d.foreignFrames[ix], foreign...) + } } } // removeOrphanRows removes all local rows with no related foreign frame func (d *joinedDataset) removeOrphanRows(local, foreign []*Frame) ([]*Frame, []*Frame, bool) { + // special edgecase where either of the buffers are empty + if len(local) == 0 || len(foreign) == 0 { + return nil, nil, true + } + // - index foreign frames based on ref value so that we have an easier time validating fIndex := make(map[string]bool) for _, ff := range foreign { diff --git a/store/rdbms/ast_transformer.go b/store/rdbms/ast_transformer.go index 882801e56..7528ee957 100644 --- a/store/rdbms/ast_transformer.go +++ b/store/rdbms/ast_transformer.go @@ -43,6 +43,12 @@ var ( // @todo IS and IS NOT; should this be calculated with eq operators? sqlExprRegistry = map[string]exprHandler{ // operators + "not": { + Args: collectParams(true, "Boolean"), + Result: wrapRes("Boolean"), + Handler: makeGenericModifierHandler("NOT"), + }, + // - bool "and": { Args: collectParams(true, "Boolean"), @@ -190,7 +196,7 @@ var ( "date": { Args: collectParams(true, "DateTime"), Result: wrapRes("Number"), - Handler: makeGenericFilterFncHandler("DATE"), + Handler: makeGenericFilterFncHandler("DateTime"), }, // generic stuff @@ -207,6 +213,22 @@ var ( }, }, + "exists": { + Args: collectParams(false, "Any"), + Result: wrapRes("Boolean"), + Handler: func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + if len(aa) != 1 { + err = fmt.Errorf("expecting 1 arguments, got %d", len(aa)) + return + } + + out = fmt.Sprintf("(%s IS NOT NULL)", aa[0].S) + selfEnclosed = true + + return + }, + }, + // - typecast "float": { Args: collectParams(true, "Any"), diff --git a/store/rdbms/ast_transformer_handlers.go b/store/rdbms/ast_transformer_handlers.go index 85cc0a66a..a9a3f9918 100644 --- a/store/rdbms/ast_transformer_handlers.go +++ b/store/rdbms/ast_transformer_handlers.go @@ -96,6 +96,21 @@ func makeGenericCompHandler(comp string) HandlerSig { } } +func makeGenericModifierHandler(mdf string) HandlerSig { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + selfEnclosed = true + + if len(aa) != 1 { + err = fmt.Errorf("expecting 1 arguments, got %d", len(aa)) + return + } + + out = fmt.Sprintf("%s %s", mdf, aa[0].S) + args = aa[0].Args + return + } +} + func makeGenericAggFncHandler(fnc string) HandlerSig { return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { selfEnclosed = true diff --git a/tests/reporter/filter_existence_test.go b/tests/reporter/filter_existence_test.go new file mode 100644 index 000000000..c51c65e59 --- /dev/null +++ b/tests/reporter/filter_existence_test.go @@ -0,0 +1,35 @@ +package reporter + +import ( + "testing" + + "github.com/cortezaproject/corteza-server/pkg/report" +) + +func Test_filter_existence(t *testing.T) { + var ( + ctx, h, s = setup(t) + m, _, dd = loadScenario(ctx, s, t, h) + ff = loadNoErrMulti(ctx, h, m, dd...) + f *report.Frame + ) + + h.a.Len(ff, 1) + + f = ff[0] + h.a.Len(f.Rows, 12) + h.a.Equal("first_name, last_name", f.Columns.String()) + checkRows(h, f, + "Maria, Königsmann", + "Ulli, Haupt", + "Engel, Loritz", + "Sascha, Jans", + "Ulli, Böhler", + "Maria, Spannagel", + "Sigi, Goldschmidt", + "Engel, Kempf", + "Maria, Krüger", + "Manu, Specht", + "Ulli, Förstner", + "Engel, Kiefer") +} diff --git a/tests/reporter/filter_negation_test.go b/tests/reporter/filter_negation_test.go new file mode 100644 index 000000000..66c361bf0 --- /dev/null +++ b/tests/reporter/filter_negation_test.go @@ -0,0 +1,32 @@ +package reporter + +import ( + "testing" + + "github.com/cortezaproject/corteza-server/pkg/report" +) + +func Test_filter_negation(t *testing.T) { + var ( + ctx, h, s = setup(t) + m, _, dd = loadScenario(ctx, s, t, h) + ff = loadNoErrMulti(ctx, h, m, dd...) + f *report.Frame + ) + + h.a.Len(ff, 1) + + f = ff[0] + h.a.Len(f.Rows, 9) + h.a.Equal("first_name, last_name", f.Columns.String()) + checkRows(h, f, + "Ulli, Haupt", + "Engel, Loritz", + "Sascha, Jans", + "Ulli, Böhler", + "Sigi, Goldschmidt", + "Engel, Kempf", + "Manu, Specht", + "Ulli, Förstner", + "Engel, Kiefer") +} diff --git a/tests/reporter/filter_temporal_test.go b/tests/reporter/filter_temporal_test.go new file mode 100644 index 000000000..9c682af45 --- /dev/null +++ b/tests/reporter/filter_temporal_test.go @@ -0,0 +1,35 @@ +package reporter + +import ( + "testing" + + "github.com/cortezaproject/corteza-server/pkg/report" +) + +func Test_filter_temporal(t *testing.T) { + var ( + ctx, h, s = setup(t) + m, _, dd = loadScenario(ctx, s, t, h) + ff = loadNoErrMulti(ctx, h, m, dd...) + f *report.Frame + ) + + h.a.Len(ff, 1) + + f = ff[0] + h.a.Len(f.Rows, 12) + h.a.Equal("first_name, last_name, createdAt", f.Columns.String()) + checkRows(h, f, + "Maria, Königsmann", + "Ulli, Haupt", + "Engel, Loritz", + "Sascha, Jans", + "Ulli, Böhler", + "Maria, Spannagel", + "Sigi, Goldschmidt", + "Engel, Kempf", + "Maria, Krüger", + "Manu, Specht", + "Ulli, Förstner", + "Engel, Kiefer") +} diff --git a/tests/reporter/join_empty_foreign_test.go b/tests/reporter/join_empty_foreign_test.go new file mode 100644 index 000000000..8508cd50c --- /dev/null +++ b/tests/reporter/join_empty_foreign_test.go @@ -0,0 +1,18 @@ +package reporter + +import ( + "testing" +) + +func Test_join_empty_foreign(t *testing.T) { + var ( + ctx, h, s = setup(t) + m, _, dd = loadScenario(ctx, s, t, h) + ff = loadNoErr(ctx, h, m, dd...) + ) + + h.a.Len(ff, 1) + f := ff[0] + + h.a.Equal(0, f.Size()) +} diff --git a/tests/reporter/model_describe_basic_test.go b/tests/reporter/model_describe_basic_test.go index 629320b82..77809cacc 100644 --- a/tests/reporter/model_describe_basic_test.go +++ b/tests/reporter/model_describe_basic_test.go @@ -16,7 +16,7 @@ func Test_model_describe_basic(t *testing.T) { h.a.Len(ff, 1) h.a.Equal( - "id, join_key, first_name, last_name, email, number_of_numbers, dob, createdAt, createdBy, updatedAt, updatedBy, deletedAt, deletedBy", + "id, join_key, first_name, last_name, email, number_of_numbers, dob, createdAt, createdBy, updatedAt, updatedBy, deletedAt, deletedBy", ff[0].Columns.String(), ) }) @@ -26,7 +26,7 @@ func Test_model_describe_basic(t *testing.T) { h.a.Len(ff, 1) h.a.Equal( - "id, name, type, cost, time_spent, usr, createdAt, createdBy, updatedAt, updatedBy, deletedAt, deletedBy", ff[0].Columns.String(), ) }) @@ -36,12 +36,12 @@ func Test_model_describe_basic(t *testing.T) { h.a.Len(ff, 2) h.a.Equal( - "id, join_key, first_name, last_name, email, number_of_numbers, dob, createdAt, createdBy, updatedAt, updatedBy, deletedAt, deletedBy", + "id, join_key, first_name, last_name, email, number_of_numbers, dob, createdAt, createdBy, updatedAt, updatedBy, deletedAt, deletedBy", ff.FilterByRef("users")[0].Columns.String(), ) h.a.Equal( - "id, name, type, cost, time_spent, usr, createdAt, createdBy, updatedAt, updatedBy, deletedAt, deletedBy", ff.FilterByRef("jobs")[0].Columns.String(), ) }) diff --git a/tests/reporter/testdata/filter_existence/report.json b/tests/reporter/testdata/filter_existence/report.json new file mode 100644 index 000000000..803f7090f --- /dev/null +++ b/tests/reporter/testdata/filter_existence/report.json @@ -0,0 +1,22 @@ +{ + "handle": "testing_report", + "sources": [{ + "step": { "load": { + "name": "users", + "source": "composeRecords", + "definition": { + "module": "user", + "namespace": "ns" + } + }}}], + "frames": [{ + "name": "result", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "last_name", "label": "last_name" } + ], + + "filter": "exists(first_name)" + }] +} diff --git a/tests/reporter/testdata/filter_negation/report.json b/tests/reporter/testdata/filter_negation/report.json new file mode 100644 index 000000000..e11aadeae --- /dev/null +++ b/tests/reporter/testdata/filter_negation/report.json @@ -0,0 +1,22 @@ +{ + "handle": "testing_report", + "sources": [{ + "step": { "load": { + "name": "users", + "source": "composeRecords", + "definition": { + "module": "user", + "namespace": "ns" + } + }}}], + "frames": [{ + "name": "result", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "last_name", "label": "last_name" } + ], + + "filter": "!(first_name == 'Maria')" + }] +} diff --git a/tests/reporter/testdata/filter_temporal/report.json b/tests/reporter/testdata/filter_temporal/report.json new file mode 100644 index 000000000..705715dbc --- /dev/null +++ b/tests/reporter/testdata/filter_temporal/report.json @@ -0,0 +1,23 @@ +{ + "handle": "testing_report", + "sources": [{ + "step": { "load": { + "name": "users", + "source": "composeRecords", + "definition": { + "module": "user", + "namespace": "ns" + } + }}}], + "frames": [{ + "name": "result", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "last_name", "label": "last_name" }, + { "name": "createdAt", "label": "createdAt" } + ], + + "filter": "year(createdAt) == year(now())" + }] +} diff --git a/tests/reporter/testdata/join_empty_foreign/report.json b/tests/reporter/testdata/join_empty_foreign/report.json new file mode 100644 index 000000000..a9f4b4dd5 --- /dev/null +++ b/tests/reporter/testdata/join_empty_foreign/report.json @@ -0,0 +1,37 @@ +{ + "handle": "testing_report", + "sources": [ + { "step": { "load": { + "name": "aa", + "source": "composeRecords", + "definition": { + "module": "aa", + "namespace": "ns" + } + }}}, + { "step": { "load": { + "name": "bb", + "source": "composeRecords", + "definition": { + "module": "bb", + "namespace": "ns" + }, + + "filter": "false" + }}}, + + { "step": { "join": { + "name": "joined", + + "localSource": "aa", + "localColumn": "pk", + "foreignSource": "bb", + "foreignColumn": "fk_a" + }}} + ], + + "frames": [{ + "name": "result", + "source": "joined" + }] +}