From 0558a3e397affb4880058b66adb3fa5fb64c3983 Mon Sep 17 00:00:00 2001 From: Fred Carle Date: Tue, 2 Aug 2022 00:39:21 -0400 Subject: [PATCH 1/5] add check of limit --- query/graphql/planner/limit.go | 4 +- .../query/simple/with_limit_offset_test.go | 83 +++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/query/graphql/planner/limit.go b/query/graphql/planner/limit.go index 8eab8610c8..c0a63933d9 100644 --- a/query/graphql/planner/limit.go +++ b/query/graphql/planner/limit.go @@ -59,7 +59,7 @@ func (n *hardLimitNode) Value() core.Doc { return n.plan.Value() } func (n *hardLimitNode) Next() (bool, error) { // check if we're passed the limit - if n.rowIndex-n.offset >= n.limit { + if n.limit != 0 && n.rowIndex-n.offset >= n.limit { return false, nil } @@ -139,7 +139,7 @@ func (n *renderLimitNode) Next() (bool, error) { n.currentValue = n.plan.Value() n.rowIndex++ - if n.rowIndex-n.offset > n.limit || n.rowIndex <= n.offset { + if (n.limit != 0 && n.rowIndex-n.offset > n.limit) || n.rowIndex <= n.offset { n.currentValue.Hidden = true } return true, nil diff --git a/tests/integration/query/simple/with_limit_offset_test.go b/tests/integration/query/simple/with_limit_offset_test.go index 482f3b2819..d79df00751 100644 --- a/tests/integration/query/simple/with_limit_offset_test.go +++ b/tests/integration/query/simple/with_limit_offset_test.go @@ -165,3 +165,86 @@ func TestQuerySimpleWithLimitAndOffset(t *testing.T) { executeTestCase(t, test) } } + +func TestQuerySimpleWithOffset(t *testing.T) { + tests := []testUtils.QueryTestCase{ + { + Description: "Simple query with basic offset", + Query: `query { + users(offset: 1) { + Name + Age + } + }`, + Docs: map[int][]string{ + 0: { + `{ + "Name": "John", + "Age": 21 + }`, + `{ + "Name": "Bob", + "Age": 32 + }`, + }, + }, + Results: []map[string]interface{}{ + { + "Name": "John", + "Age": uint64(21), + }, + }, + }, + { + Description: "Simple query with basic offset, more rows", + Query: `query { + users(offset: 2) { + Name + Age + } + }`, + Docs: map[int][]string{ + 0: { + `{ + "Name": "John", + "Age": 21 + }`, + `{ + "Name": "Bob", + "Age": 32 + }`, + `{ + "Name": "Carlo", + "Age": 55 + }`, + `{ + "Name": "Alice", + "Age": 19 + }`, + `{ + "Name": "Melynda", + "Age": 30 + }`, + }, + }, + Results: []map[string]interface{}{ + { + "Name": "Alice", + "Age": uint64(19), + }, + { + "Name": "John", + "Age": uint64(21), + }, + { + "Name": "Carlo", + "Age": uint64(55), + }, + }, + }, + } + + for _, test := range tests { + executeTestCase(t, test) + } +} From adf94eea7a22420dcb49deb58f4a2a58b811c0f7 Mon Sep 17 00:00:00 2001 From: Fred Carle Date: Tue, 2 Aug 2022 00:53:11 -0400 Subject: [PATCH 2/5] add limit check on explain --- query/graphql/planner/limit.go | 22 ++++++++++++++----- .../query/explain/with_hard_limit_test.go | 2 -- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/query/graphql/planner/limit.go b/query/graphql/planner/limit.go index c0a63933d9..3a840529fe 100644 --- a/query/graphql/planner/limit.go +++ b/query/graphql/planner/limit.go @@ -82,10 +82,15 @@ func (n *hardLimitNode) Next() (bool, error) { func (n *hardLimitNode) Source() planNode { return n.plan } func (n *hardLimitNode) Explain() (map[string]interface{}, error) { - return map[string]interface{}{ - limitLabel: n.limit, + exp := map[string]interface{}{ offsetLabel: n.offset, - }, nil + } + + if n.limit != 0 { + exp[limitLabel] = n.limit + } + + return exp, nil } // limit the results, flagging any records outside the bounds of limit/offset with @@ -148,8 +153,13 @@ func (n *renderLimitNode) Next() (bool, error) { func (n *renderLimitNode) Source() planNode { return n.plan } func (n *renderLimitNode) Explain() (map[string]interface{}, error) { - return map[string]interface{}{ - limitLabel: n.limit, + exp := map[string]interface{}{ offsetLabel: n.offset, - }, nil + } + + if n.limit != 0 { + exp[limitLabel] = n.limit + } + + return exp, nil } diff --git a/tests/integration/query/explain/with_hard_limit_test.go b/tests/integration/query/explain/with_hard_limit_test.go index ada9b3618f..3098d6bab9 100644 --- a/tests/integration/query/explain/with_hard_limit_test.go +++ b/tests/integration/query/explain/with_hard_limit_test.go @@ -138,7 +138,6 @@ func TestExplainQueryWithOnlyOffsetSpecified(t *testing.T) { "explain": dataMap{ "selectTopNode": dataMap{ "hardLimitNode": dataMap{ - "limit": int64(0), "offset": int64(2), "selectNode": dataMap{ "filter": nil, @@ -528,7 +527,6 @@ func TestExplainQueryWithOnlyOffsetOnChild(t *testing.T) { "subType": dataMap{ "selectTopNode": dataMap{ "hardLimitNode": dataMap{ - "limit": int64(0), "offset": int64(2), "selectNode": dataMap{ "filter": nil, From 3e3bae5482c117d24971fbac406f358a74d7ccf3 Mon Sep 17 00:00:00 2001 From: Fred Carle Date: Tue, 2 Aug 2022 12:54:54 -0400 Subject: [PATCH 3/5] revert to always including limit --- query/graphql/planner/limit.go | 22 +++++-------------- .../query/explain/with_hard_limit_test.go | 2 ++ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/query/graphql/planner/limit.go b/query/graphql/planner/limit.go index 3a840529fe..c0a63933d9 100644 --- a/query/graphql/planner/limit.go +++ b/query/graphql/planner/limit.go @@ -82,15 +82,10 @@ func (n *hardLimitNode) Next() (bool, error) { func (n *hardLimitNode) Source() planNode { return n.plan } func (n *hardLimitNode) Explain() (map[string]interface{}, error) { - exp := map[string]interface{}{ + return map[string]interface{}{ + limitLabel: n.limit, offsetLabel: n.offset, - } - - if n.limit != 0 { - exp[limitLabel] = n.limit - } - - return exp, nil + }, nil } // limit the results, flagging any records outside the bounds of limit/offset with @@ -153,13 +148,8 @@ func (n *renderLimitNode) Next() (bool, error) { func (n *renderLimitNode) Source() planNode { return n.plan } func (n *renderLimitNode) Explain() (map[string]interface{}, error) { - exp := map[string]interface{}{ + return map[string]interface{}{ + limitLabel: n.limit, offsetLabel: n.offset, - } - - if n.limit != 0 { - exp[limitLabel] = n.limit - } - - return exp, nil + }, nil } diff --git a/tests/integration/query/explain/with_hard_limit_test.go b/tests/integration/query/explain/with_hard_limit_test.go index 3098d6bab9..ada9b3618f 100644 --- a/tests/integration/query/explain/with_hard_limit_test.go +++ b/tests/integration/query/explain/with_hard_limit_test.go @@ -138,6 +138,7 @@ func TestExplainQueryWithOnlyOffsetSpecified(t *testing.T) { "explain": dataMap{ "selectTopNode": dataMap{ "hardLimitNode": dataMap{ + "limit": int64(0), "offset": int64(2), "selectNode": dataMap{ "filter": nil, @@ -527,6 +528,7 @@ func TestExplainQueryWithOnlyOffsetOnChild(t *testing.T) { "subType": dataMap{ "selectTopNode": dataMap{ "hardLimitNode": dataMap{ + "limit": int64(0), "offset": int64(2), "selectNode": dataMap{ "filter": nil, From 59588d45aecc0fc050abde8ecb948d7b73bc81d3 Mon Sep 17 00:00:00 2001 From: Fred Carle Date: Tue, 2 Aug 2022 12:58:50 -0400 Subject: [PATCH 4/5] apply suggestion --- tests/integration/query/simple/with_limit_offset_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/query/simple/with_limit_offset_test.go b/tests/integration/query/simple/with_limit_offset_test.go index d79df00751..0e807facba 100644 --- a/tests/integration/query/simple/with_limit_offset_test.go +++ b/tests/integration/query/simple/with_limit_offset_test.go @@ -169,7 +169,7 @@ func TestQuerySimpleWithLimitAndOffset(t *testing.T) { func TestQuerySimpleWithOffset(t *testing.T) { tests := []testUtils.QueryTestCase{ { - Description: "Simple query with basic offset", + Description: "Simple query with offset only", Query: `query { users(offset: 1) { Name @@ -196,7 +196,7 @@ func TestQuerySimpleWithOffset(t *testing.T) { }, }, { - Description: "Simple query with basic offset, more rows", + Description: "Simple query with offset only, more rows", Query: `query { users(offset: 2) { Name From 9d772b24b9490a4c984598b770d1f32deae00dc6 Mon Sep 17 00:00:00 2001 From: Fred Carle Date: Tue, 2 Aug 2022 13:56:46 -0400 Subject: [PATCH 5/5] change limit 0 to nil --- query/graphql/planner/limit.go | 20 +++++++++++++++---- .../query/explain/with_hard_limit_test.go | 4 ++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/query/graphql/planner/limit.go b/query/graphql/planner/limit.go index c0a63933d9..21ae9acff6 100644 --- a/query/graphql/planner/limit.go +++ b/query/graphql/planner/limit.go @@ -82,10 +82,16 @@ func (n *hardLimitNode) Next() (bool, error) { func (n *hardLimitNode) Source() planNode { return n.plan } func (n *hardLimitNode) Explain() (map[string]interface{}, error) { - return map[string]interface{}{ + exp := map[string]interface{}{ limitLabel: n.limit, offsetLabel: n.offset, - }, nil + } + + if n.limit == 0 { + exp[limitLabel] = nil + } + + return exp, nil } // limit the results, flagging any records outside the bounds of limit/offset with @@ -148,8 +154,14 @@ func (n *renderLimitNode) Next() (bool, error) { func (n *renderLimitNode) Source() planNode { return n.plan } func (n *renderLimitNode) Explain() (map[string]interface{}, error) { - return map[string]interface{}{ + exp := map[string]interface{}{ limitLabel: n.limit, offsetLabel: n.offset, - }, nil + } + + if n.limit == 0 { + exp[limitLabel] = nil + } + + return exp, nil } diff --git a/tests/integration/query/explain/with_hard_limit_test.go b/tests/integration/query/explain/with_hard_limit_test.go index ada9b3618f..55ea103ec0 100644 --- a/tests/integration/query/explain/with_hard_limit_test.go +++ b/tests/integration/query/explain/with_hard_limit_test.go @@ -138,7 +138,7 @@ func TestExplainQueryWithOnlyOffsetSpecified(t *testing.T) { "explain": dataMap{ "selectTopNode": dataMap{ "hardLimitNode": dataMap{ - "limit": int64(0), + "limit": nil, "offset": int64(2), "selectNode": dataMap{ "filter": nil, @@ -528,7 +528,7 @@ func TestExplainQueryWithOnlyOffsetOnChild(t *testing.T) { "subType": dataMap{ "selectTopNode": dataMap{ "hardLimitNode": dataMap{ - "limit": int64(0), + "limit": nil, "offset": int64(2), "selectNode": dataMap{ "filter": nil,