cascade directive at subqueries #4006

animesh2049 · 2019-09-17T14:10:21Z

This change is

related to #1755

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@animesh2049 you can click here to see the review status or cancel the code review job.

pullrequest

	1 Message
✅	~~The description of this pull request is blank. Adding a high-level summary will help our reviewers provide better feedback.~~

mangalaman93 · 2019-09-17T14:54:59Z

query/common_test.go

+		panic(fmt.Sprintf("Could not perform DropAll op. Got error %v", err.Error()))
+	}
+
+	schema := `


fix the formatting!

mangalaman93 · 2019-09-17T14:55:19Z

query/common_test.go

+                  xname: string @index(term, exact) .
+                  xage: int .
+                  xfriend: [uid] .
+`


don't leave any newlines here after multiline string

mangalaman93 · 2019-09-17T14:55:29Z

query/common_test.go

+	setSchema(schema)
+
+	data := `
+                _:animesh <xname> "Animesh" .


mangalaman93 · 2019-09-17T14:55:47Z

query/query0_test.go

@@ -2287,5 +2287,6 @@ func TestMain(m *testing.M) {
 	client = testutil.DgraphClientWithGroot(testutil.SockAddr)

 	populateCluster()
+	insertSmallDataSet()


call it populateCascadeDataset()

mangalaman93 · 2019-09-17T14:56:13Z

query/common_test.go

@@ -618,3 +618,31 @@ func populateCluster() {
 		<307> <updated_at> "2019-05-28" (modified_at=2019-03-24T14:41:57+05:30) .
 	`)
 }
+
+func insertSmallDataSet() {
+	err := client.Alter(context.Background(), &api.Operation{DropAll: true})


Should not call dropall here, otherwise existing test will fail

mangalaman93 · 2019-09-17T14:56:28Z

query/query4_test.go

+                              }
+                          }
+                      }
+


extra newline

mangalaman93 · 2019-09-17T14:56:33Z

query/query4_test.go

+                              }
+                          }
+                      }
+


pullrequest

The additional feedback outside of the existing comments are focused on readability.

Reviewed with ❤️ by PullRequest

pullrequest · 2019-09-17T15:56:57Z

query/common_test.go

@@ -618,3 +618,31 @@ func populateCluster() {
 		<307> <updated_at> "2019-05-28" (modified_at=2019-03-24T14:41:57+05:30) .
 	`)
 }
+
+func insertSmallDataSet() {
+	err := client.Alter(context.Background(), &api.Operation{DropAll: true})


I think the syntax here should change to reflect the struct initialization and obtaining a pointer value.

o := api.Operation{DropAll: true} err := client.Alter(context.Background(), &o)

It is an additional line, but it certainly makes the code more readable in terms of what operations are occurring.

martinmr

Reviewed 4 of 5 files at r1.
Reviewable status: 4 of 5 files reviewed, 9 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, and @pawanrawal)

query/common_test.go, line 623 at r1 (raw file):

Previously, pullrequest[bot] wrote…

I think the syntax here should change to reflect the struct initialization and obtaining a pointer value.
o := api.Operation{DropAll: true}
err := client.Alter(context.Background(), &o)
It is an additional line, but it certainly makes the code more readable in terms of what operations are occurring.

Right now all the data for tests is added in the populateCluster method. Not great since new data could potentially break existing tests but it's better than removing data in each test.

query/query0_test.go, line 2290 at r1 (raw file):

	populateCluster()
	insertSmallDataSet()

remove this function and move all the data insertion to populateCluster. We shouldn't have multiple places where data is inserted. We are already doing this for facet tests and it's kind of a pain in the ass.

query/query4_test.go, line 23 at r1 (raw file):

Quoted 4 lines of code…

	// "encoding/json"
	// "fmt"
	// "strings"
func TestTypeExpandLang(t *testing.T) { ⋯ expand 9 lines

can you remove the commented imports?

mangalaman93

Two comments -

Fix the JSON formatting, doesn't look good
Add a test for multiple cascade in a query

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @animesh2049, @manishrjain, @martinmr, and @pawanrawal)

animesh2049

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)

query/query0_test.go, line 2290 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

remove this function and move all the data insertion to populateCluster. We shouldn't have multiple places where data is inserted. We are already doing this for facet tests and it's kind of a pain in the ass.

Done.

query/query4_test.go, line 23 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	// "encoding/json"
	// "fmt"
	// "strings"
func TestTypeExpandLang(t *testing.T) { ⋯ expand 9 lines

can you remove the commented imports?

Done.

pawanrawal

Good work. I only have two suggestions, make tests start with multiple source uids to make them representative of the general use-case and add some comments to them to make it easier for the reviewer and the person looking at them later.

Reviewed 2 of 5 files at r1, 2 of 3 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

query/query4_test.go, line 341 at r2 (raw file):

}

func TestCascadeSubQuery1(t *testing.T) {

Can you add a comment about this test. In general it would be nice to explain your tests or make the name more descriptive than SubQuery1, SubQuery2 so that others who look at it later know what the intention of the test is.

I am assuming that Michonne has some friends who don't have a full_name and hence they are removed? Or is it that Michonne's friends friend don't have all the fields name, full_name ...?

query/query4_test.go, line 344 at r2 (raw file):

	query := `
	{
		me(func: uid(0x01)) {

Could you update your tests to start with more than one source uid, have atleast three.

query/query4_test.go, line 407 at r2 (raw file):

						"friend": [
							{
								"name": "Rick Grimes",

I am assuming Rick has more friends but only Michonne is the friend with all the fields? If yes, then please explain in a comment.

query/query4_test.go, line 433 at r2 (raw file):

}

func TestCascadeSubQuery3(t *testing.T) {

TestCascadeRepeatedMultipleLevels(t *testing.T) and add a comment saying that it should be the same as only applying it at the top level friend.

animesh2049

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)

query/query4_test.go, line 341 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can you add a comment about this test. In general it would be nice to explain your tests or make the name more descriptive than SubQuery1, SubQuery2 so that others who look at it later know what the intention of the test is.

I am assuming that Michonne has some friends who don't have a full_name and hence they are removed? Or is it that Michonne's friends friend don't have all the fields name, full_name ...?

Done.

query/query4_test.go, line 344 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Could you update your tests to start with more than one source uid, have atleast three.

Done.

query/query4_test.go, line 407 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I am assuming Rick has more friends but only Michonne is the friend with all the fields? If yes, then please explain in a comment.

Added an extra test with explanation.

query/query4_test.go, line 433 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

TestCascadeRepeatedMultipleLevels(t *testing.T) and add a comment saying that it should be the same as only applying it at the top level friend.

Done.

animesh2049 added 2 commits September 17, 2019 17:15

Add support for cascade for subqueries

6a62602

Add tests for @cascade at subqueries

5b20d41

animesh2049 requested review from manishrjain, pawanrawal and a team as code owners September 17, 2019 14:10

pullrequest bot reviewed Sep 17, 2019

View reviewed changes

mangalaman93 requested changes Sep 17, 2019

View reviewed changes

pullrequest bot reviewed Sep 17, 2019

View reviewed changes

martinmr suggested changes Sep 17, 2019

View reviewed changes

animesh2049 added 3 commits September 19, 2019 16:11

Extend cascade tests in query package

741b349

Remove unused imports

2826d47

Add more test and format output

aad574f

mangalaman93 changed the title ~~[WIP] cascade directive at subqueries~~ cascade directive at subqueries Sep 19, 2019

mangalaman93 requested a review from martinmr September 19, 2019 12:13

mangalaman93 approved these changes Sep 19, 2019

View reviewed changes

Add test for multiple cascade

48789a6

animesh2049 commented Sep 19, 2019

View reviewed changes

Format output

80dc302

mangalaman93 approved these changes Sep 19, 2019

View reviewed changes

pawanrawal suggested changes Sep 20, 2019

View reviewed changes

Address PR comments

2c0b1a7

animesh2049 commented Sep 20, 2019

View reviewed changes

animesh2049 merged commit 7d925b2 into master Sep 20, 2019

animesh2049 deleted the animesh2049/cascade_directive branch September 26, 2019 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cascade directive at subqueries #4006

cascade directive at subqueries #4006

animesh2049 commented Sep 17, 2019 •

edited by MichelDiz

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

mangalaman93 Sep 17, 2019

mangalaman93 Sep 17, 2019

mangalaman93 Sep 17, 2019

mangalaman93 Sep 17, 2019

mangalaman93 Sep 17, 2019

mangalaman93 Sep 17, 2019

mangalaman93 Sep 17, 2019

pullrequest bot left a comment

pullrequest bot Sep 17, 2019

martinmr left a comment

mangalaman93 left a comment

animesh2049 left a comment

pawanrawal left a comment

animesh2049 left a comment

cascade directive at subqueries #4006

cascade directive at subqueries #4006

Conversation

animesh2049 commented Sep 17, 2019 • edited by MichelDiz Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

mangalaman93 Sep 17, 2019

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Sep 17, 2019

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

animesh2049 commented Sep 17, 2019 •

edited by MichelDiz

Loading

pullrequest bot left a comment •

edited

Loading