fix: Add a check to ensure limit is not 0 when evaluating query limit and offset #706

fredcarle · 2022-08-02T04:43:12Z

Relevant issue(s)

Resolves #705
Resolves #620

Description

This PR solves a situation where setting an offset to the query without setting a limit would yield a single result.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

integration testing

Specify the platform(s) on which this was tested:

MacOS

fredcarle · 2022-08-02T04:46:35Z

@jsimnz the fix is a little different than what you suggested. Decided on not changing the node limit directly and rather just check that it's not 0. This avoids the large int64 in the explain response.

jsimnz · 2022-08-02T05:38:29Z

@jsimnz the fix is a little different than what you suggested. Decided on not changing the node limit directly and rather just check that it's not 0. This avoids the large int64 in the explain response.

makes sense

jsimnz

LGTM

shahzadlone · 2022-08-02T08:08:19Z

query/graphql/planner/limit.go

-		limitLabel:  n.limit,
+	exp := map[string]interface{}{
 		offsetLabel: n.offset,
-	}, nil
+	}
+
+	if n.limit != 0 {
+		exp[limitLabel] = n.limit
+	}
+
+	return exp, nil


suggestion(blocking): I disagree with this change. Every implementation of Explain right now dumps a fixed number of attributes. n.limit attribute is the most important attribute in both the "limitNodes". Furthermore the current implementation of explain is also considered the v1(i.e. verbose=true) and if we want to have a different number of attributes depending on certain situations those need to be first thought out for every explain node and be handled in a verbose = false feature. Please revert this change.

shahzadlone · 2022-08-02T08:09:41Z

query/graphql/planner/limit.go

-		limitLabel:  n.limit,
+	exp := map[string]interface{}{
 		offsetLabel: n.offset,
-	}, nil
+	}
+
+	if n.limit != 0 {
+		exp[limitLabel] = n.limit
+	}
+
+	return exp, nil
 }


suggestion(blocking): Similar comment as above.

shahzadlone · 2022-08-02T08:12:11Z

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),


suggestion(blocking): Following the above comments is an important attribute, please leave in.

shahzadlone · 2022-08-02T08:12:19Z

tests/integration/query/explain/with_hard_limit_test.go

@@ -528,7 +527,6 @@ func TestExplainQueryWithOnlyOffsetOnChild(t *testing.T) {
 								"subType": dataMap{
 									"selectTopNode": dataMap{
 										"hardLimitNode": dataMap{
-											"limit":  int64(0),


suggestion(blocking): Following the above comments is an important attribute, please leave in.

shahzadlone · 2022-08-02T08:13:52Z

tests/integration/query/simple/with_limit_offset_test.go

+func TestQuerySimpleWithOffset(t *testing.T) {
+	tests := []testUtils.QueryTestCase{
+		{
+			Description: "Simple query with basic offset",


nitpick:

Suggested change

func TestQuerySimpleWithOffset(t *testing.T) {

tests := []testUtils.QueryTestCase{

{

Description: "Simple query with basic offset",

func TestQuerySimpleWithOnlyOffset(t *testing.T) {

tests := []testUtils.QueryTestCase{

{

Description: "Simple query with only offset",

shahzadlone · 2022-08-02T08:16:07Z

tests/integration/query/simple/with_limit_offset_test.go

+			},
+		},
+		{
+			Description: "Simple query with basic offset, more rows",


nitpick: basic seems generic to me, perhaps use of only can signal that you are testing the case without limit.

Suggested change

Description: "Simple query with basic offset, more rows",

Description: "Simple query with only offset, more rows",

shahzadlone

Thanks for the quick solution. Mainly concerned about the explain deleting the n.limit attribute in certain conditions mostly because current explain should dump attribute of interest as is.

shahzadlone

LGTM!

Don't forget to update the PR Description:

This also removes the "limit": 0 object from the explain response when there is no limit.

codecov · 2022-08-02T18:07:00Z

Codecov Report

Merging #706 (9d772b2) into develop (b17b020) will increase coverage by 0.02%.
The diff coverage is 81.25%.

@@             Coverage Diff             @@
##           develop     #706      +/-   ##
===========================================
+ Coverage    57.39%   57.42%   +0.02%     
===========================================
  Files          143      143              
  Lines        16398    16408      +10     
===========================================
+ Hits          9412     9422      +10     
  Misses        6102     6102              
  Partials       884      884

Impacted Files	Coverage Δ
query/graphql/planner/limit.go	`90.69% <81.25%> (+1.22%)`	⬆️

… and offset (sourcenetwork#706) Relevant issue(s) Resolves sourcenetwork#705 Description This PR solves a situation where setting an offset to the query without setting a limit would yield a single result.

fredcarle added action/no-benchmark Skips the action that runs the benchmark. area/planner Related to the planner system labels Aug 2, 2022

fredcarle added this to the DefraDB v0.3 milestone Aug 2, 2022

fredcarle requested a review from a team August 2, 2022 04:43

fredcarle self-assigned this Aug 2, 2022

fredcarle added the bug Something isn't working label Aug 2, 2022

jsimnz approved these changes Aug 2, 2022

View reviewed changes

shahzadlone reviewed Aug 2, 2022

View reviewed changes

shahzadlone requested changes Aug 2, 2022

View reviewed changes

fredcarle added 5 commits August 2, 2022 13:57

add check of limit

0558a3e

add limit check on explain

adf94ee

revert to always including limit

3e3bae5

apply suggestion

59588d4

change limit 0 to nil

9d772b2

fredcarle force-pushed the fredcarle/fix/I705-offset-with-no-limit branch from 5f2142c to 9d772b2 Compare August 2, 2022 17:57

shahzadlone approved these changes Aug 2, 2022

View reviewed changes

fredcarle merged commit 14c5b14 into develop Aug 2, 2022

fredcarle deleted the fredcarle/fix/I705-offset-with-no-limit branch August 2, 2022 18:09

fredcarle mentioned this pull request Aug 5, 2022

No tests for offset only #620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add a check to ensure limit is not 0 when evaluating query limit and offset #706

fix: Add a check to ensure limit is not 0 when evaluating query limit and offset #706

fredcarle commented Aug 2, 2022 •

edited

Loading

fredcarle commented Aug 2, 2022

jsimnz commented Aug 2, 2022

jsimnz left a comment

shahzadlone Aug 2, 2022

shahzadlone Aug 2, 2022

shahzadlone Aug 2, 2022

shahzadlone Aug 2, 2022

shahzadlone Aug 2, 2022

shahzadlone Aug 2, 2022 •

edited

Loading

shahzadlone left a comment

shahzadlone left a comment •

edited

Loading

codecov bot commented Aug 2, 2022 •

edited

Loading

	Description: "Simple query with basic offset, more rows",
	Description: "Simple query with only offset, more rows",

fix: Add a check to ensure limit is not 0 when evaluating query limit and offset #706

fix: Add a check to ensure limit is not 0 when evaluating query limit and offset #706

Conversation

fredcarle commented Aug 2, 2022 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

fredcarle commented Aug 2, 2022

jsimnz commented Aug 2, 2022

jsimnz left a comment

Choose a reason for hiding this comment

shahzadlone Aug 2, 2022

Choose a reason for hiding this comment

shahzadlone Aug 2, 2022

Choose a reason for hiding this comment

shahzadlone Aug 2, 2022

Choose a reason for hiding this comment

shahzadlone Aug 2, 2022

Choose a reason for hiding this comment

shahzadlone Aug 2, 2022

Choose a reason for hiding this comment

shahzadlone Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2022 • edited Loading

Codecov Report

fredcarle commented Aug 2, 2022 •

edited

Loading

shahzadlone Aug 2, 2022 •

edited

Loading

shahzadlone left a comment •

edited

Loading

codecov bot commented Aug 2, 2022 •

edited

Loading