Added Support for val function inside Upsert

[harshil-goel]

Added VAL function to Upsert mutations.
This allows the user to do queries like

// Bulk update a query
upsert {
  query {
    u as var(func: has(amount)) {
      amt as amount 
    }
    me () {
      updated_amt as math(amt+1)
    }
  }

// Set variables to an aggregate variable
upsert {
  query {
    u as var(func: has(amount)) {
      amt as amount 
    }
    me() {
      max_amt as max(val(amt))
    }
  }
  mutation {
    set {
      uid(u) <amount> val(max_amt) .
    }
  }
}

// Delete specific values
upsert {
  query {
    u as var(func: has(amount)) {
      amt as amount
    }
  }
  
  mutation {
    delete {
      uid(u) <amount> val(amt) .
    }
  }
}

---

This change is 

[pullrequest]

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

---

@harshil-goel you can click here to see the review status or cancel the code review job.

[CLAassistant]

All committers have signed the CLA.

[golangcibot]

line is 110 characters (from lll)

[pullrequest]

⚠️ Warning

PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More

[golangcibot]

line is 102 characters (from lll)

[manishrjain]

@pawanrawal and @mangalaman93 , please review this. Once done, let me know and I'll review.

[mangalaman93]

Need more testing, should add support for val in delete mutations as well. We should also consider adding support for math, increases the power of val a lot more.

[mangalaman93]

We are not doing what this states in the test

[mangalaman93]

not formatted correctly

[mangalaman93]

please format them starting column 0. remove the spaces

[mangalaman93]

same, move everything to one tab left

[mangalaman93]

Keep the predicate here as <amount>

[mangalaman93]

instead of stop it from execution, you could write ignore the mutation silently

[mangalaman93]

I'm not sure whether it is accepted to write string value here. Does this work for other types as well?

[mangalaman93]

You could just do gmu.Set = gmuSet

[mangalaman93]

Also, what happened to updating gmu.Del mutations? Those could contain val function as well.

[pullrequest]

Left a few best practice nitpicks and a comment on likely incorrect printf operand inline.

---

Reviewed with ❤️ by PullRequest

[pullrequest]

I think you may want to use %c here instead of %s since this is a rune.

[pullrequest]

minor nitpick: but would use expectedRes here instead of an underscore for proper Go naming

[pullrequest]

I believe the len check here should be sufficient without the v.Vals == nil

[pullrequest]

may want to update the docstring for this method to the name name as well

[pullrequest]

Could potentially use a const for val( and use len(valConst) here instead of hard-coding 4 in case this changes at some point.

[pullrequest]

could add capacity to this make

vals := make(map[string]string, len(v.Vals))

[mangalaman93]

use ok` instead.

[harshil-goel]

Reviewable status: 0 of 4 files reviewed, 24 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

chunker/rdf_state.go, line 152 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

Done.

---

chunker/rdf_state.go, line 141 at r4 (raw file):

Previously, pullrequest[bot] wrote…

I think you may want to use %c here instead of %s since this is a rune.

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1409 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

We are not doing what this states in the test

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1411 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

please format them starting column 0. remove the spaces

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1431 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

not formatted correctly

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1473 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

same, move everything to one tab left

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1482 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Keep the predicate here as <amount>

Done.

---

edgraph/server.go, line 725 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 110 characters (from lll)

Done.

---

edgraph/server.go, line 717 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 102 characters (from lll)

Done.

---

edgraph/server.go, line 657 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Not sure why VAL is capital, could just be varToVal

Done.

---

edgraph/server.go, line 659 at r4 (raw file):

Previously, pullrequest[bot] wrote…

I believe the len check here should be sufficient without the v.Vals == nil

Done.

---

edgraph/server.go, line 662 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

please add a comment here as what we are storing in varToVal. An example would be useful too.

Done.

---

edgraph/server.go, line 663 at r4 (raw file):

Previously, pullrequest[bot] wrote…

could add capacity to this make

vals := make(map[string]string, len(v.Vals))

Done.

---

edgraph/server.go, line 684 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

could just be updateValInMutations and updateUIDInMutations (notice the s in both)

Done.

---

edgraph/server.go, line 718 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

object will VAL?

Done.

---

edgraph/server.go, line 724 at r4 (raw file):

Previously, pullrequest[bot] wrote…

Could potentially use a const for val( and use len(valConst) here instead of hard-coding 4 in case this changes at some point.

Done.

---

edgraph/server.go, line 728 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Do we really need to return two values from here?

Done.

---

edgraph/server.go, line 745 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I'm not sure whether it is accepted to write string value here. Does this work for other types as well?

Done.

---

edgraph/server.go, line 753 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

You could just do gmu.Set = gmuSet

Done.

---

edgraph/server.go, line 755 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Also, what happened to updating gmu.Del mutations? Those could contain val function as well.

Done.

---

edgraph/server.go, line 760 at r4 (raw file):

Previously, pullrequest[bot] wrote…

may want to update the docstring for this method to the name name as well

Done.

---

edgraph/server.go, line 740 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

use ok` instead.

Done.

[harshil-goel]

Reviewable status: 0 of 4 files reviewed, 23 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

edgraph/server.go, line 720 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

instead of stop it from execution, you could write ignore the mutation silently

Done.

[pawanrawal]

The PR could use a longer description to tell us about what new use cases does it now enable. From what I could understand it enables two cases, that is setting a value defined in another aggregate variable and deleting values for a uid without knowing them.

Reviewable status: 0 of 4 files reviewed, 34 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/upsert_test.go, line 1430 at r6 (raw file):

  mutation {
    set {
      uid(u ) <amount> val(amt) .

uid(u)

---

dgraph/cmd/alpha/upsert_test.go, line 1432 at r6 (raw file):

      uid(u ) <amount> val(amt) .
      uid(u) <name> val (n) .
      uid(u) <branch> val( b) .

val(b)

---

dgraph/cmd/alpha/upsert_test.go, line 1437 at r6 (raw file):

      uid(u) <openDate> val(open) .
      uid(u) <password> val(pass) .
      uid(u) <loc> val(l) .

Modify the test case so that one the entities doesn't have a value for a variable. For e.g. don't set age and active for user3. This would test if the code works fine when values don't exist corresponding to an uid.

---

dgraph/cmd/alpha/upsert_test.go, line 1561 at r6 (raw file):

  mutation {
    set {
      uid(u) <amount> val(amt) .

All these tests store a value corresponding to the uid in the variable and then use the same value to update, thereby producing no change the result before and after the upsert which makes we wonder what do we get by supporting this?

---

dgraph/cmd/alpha/upsert_test.go, line 1584 at r6 (raw file):

  mutation {
    set {
      uid(u) <amount> val(max_amt) .

This seems to be the only new use case supported by this feature to support updating value for a uid which was defined in another aggregate variable.

---

dgraph/cmd/alpha/upsert_test.go, line 1611 at r6 (raw file):

   }
}`
	testutil.CompareJSON(t, res, expectedRes)

Add a testcase for delete, that seems be a valid use case where I might not know the value but still would want to delete it for a user.

---

edgraph/server.go, line 660 at r6 (raw file):

	// Result will either be a map from uid to value of that variable or the
	// variable can be an aggregate variable, in which case the result is
	// stored in key 0

I don't see much point in all this transformation. We have this information in qr.Vars already. The only difference being that uids are stored as uint64 there instead of strings. Shouldn't we pass qr.Vars directly to the functions updateUIDinMutations and the other one and do the uint to string conversion later?

---

edgraph/server.go, line 725 at r6 (raw file):

// If val(variable) exists in a query, but the values are not there for the variable,
// it will delete the mutation itself, and ignore the mutation silently
func updateVALinMutations(gmu *gql.Mutation, varToVal map[string]map[string]types.Val) {

Its not very clear here what map[string]map[string]types.Val is. Perhaps you could look into defining a type like

type uidToValue map[string]types.Val

and then you could have

map[string]uidToValue

But you could just work with req.Vars instead as well.

---

edgraph/server.go, line 745 at r6 (raw file):

			continue
		}
		val, ok := newObs[nq.Subject]

What is newObs, could you please use a more descriptive name?

---

edgraph/server.go, line 751 at r6 (raw file):

			val, ok = newObs["0"]
			if !ok {
				//Value doesn't exist, ignore mutation

have a space between // and the starting of the sentence everywhere.

---

edgraph/server.go, line 768 at r6 (raw file):

	gmuSet := gmu.Set[:0]
	for _, nq := range gmu.Set {

This block looks really identical to the one above for gmu.Del, could we have a mini function and call them for both?

[harshil-goel]

Reviewable status: 0 of 4 files reviewed, 34 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/upsert_test.go, line 1430 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

uid(u)

UID(u ) is for validating that the RDF is being parsed properly.

---

dgraph/cmd/alpha/upsert_test.go, line 1432 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

val(b)

val( b) is for validating that the RDF is being parsed properly.

---

dgraph/cmd/alpha/upsert_test.go, line 1437 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Modify the test case so that one the entities doesn't have a value for a variable. For e.g. don't set age and active for user3. This would test if the code works fine when values don't exist corresponding to an uid.

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1561 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

All these tests store a value corresponding to the uid in the variable and then use the same value to update, thereby producing no change the result before and after the upsert which makes we wonder what do we get by supporting this?

I have updated the test cases to make a modification in the value, for example like adding interest in the banking case.

---

dgraph/cmd/alpha/upsert_test.go, line 1584 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This seems to be the only new use case supported by this feature to support updating value for a uid which was defined in another aggregate variable.

I have updated the test cases to make a modification in the value, for example like adding interest in the banking case.

---

dgraph/cmd/alpha/upsert_test.go, line 1611 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a testcase for delete, that seems be a valid use case where I might not know the value but still would want to delete it for a user.

Done.

---

edgraph/server.go, line 660 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I don't see much point in all this transformation. We have this information in qr.Vars already. The only difference being that uids are stored as uint64 there instead of strings. Shouldn't we pass qr.Vars directly to the functions updateUIDinMutations and the other one and do the uint to string conversion later?

Done.

---

edgraph/server.go, line 725 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Its not very clear here what map[string]map[string]types.Val is. Perhaps you could look into defining a type like

type uidToValue map[string]types.Val

and then you could have

map[string]uidToValue

But you could just work with req.Vars instead as well.

Done.

---

edgraph/server.go, line 745 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What is newObs, could you please use a more descriptive name?

Done.

---

edgraph/server.go, line 751 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

have a space between // and the starting of the sentence everywhere.

Done.

---

edgraph/server.go, line 768 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This block looks really identical to the one above for gmu.Del, could we have a mini function and call them for both?

Done.

[mangalaman93]

Reviewable status: 0 of 4 files reviewed, 24 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

chunker/rdf_state.go, line 139 at r7 (raw file):

			}

		case r == 'u' || r == 'v':

Add a comment here why just 'u` and 'v'?

---

dgraph/cmd/alpha/upsert_test.go, line 1629 at r7 (raw file):

  mutation {
    delete {
      uid(u) <amount> val(amt) .

Does it not delete the RDF when the object is different than val(amt) i.e doesn't match the stored value?

---

dgraph/cmd/alpha/upsert_test.go, line 1655 at r7 (raw file):

  }
}`
	_, _, _, err = mutationWithTs(m4, "application/rdf", false, true, 0)

Add test case for when Val is used in Subject.

---

edgraph/server.go, line 706 at r7 (raw file):

// If val(variable) exists in a query, but the values are not there for the variable,
// it will delete the mutation itself, and ignore the mutation silently
func updateValInMutations(gmu *gql.Mutation, varToVal query.Request) {

query.Request should just be named req or something
And, also I think it makes sense to export the type of req.Vars because we are clearly using it outside the package where it is defined.

[pawanrawal]

Its getting there. One more iteration and should be good to go. Let me know when you have addressed the comments and I'll have another look.

Reviewable status: 0 of 4 files reviewed, 16 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/upsert_test.go, line 1629 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Does it not delete the RDF when the object is different than val(amt) i.e doesn't match the stored value?

It shouldn't delete the value in that case. Can we add a testcase for this as well? Basically store the variable as

me () {
      updated_amt as math(amt+1)

And then use this in the delete, there should not be any change in the result.

---

dgraph/cmd/alpha/upsert_test.go, line 1655 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Add test case for when Val is used in Subject.

Yeah that's a good one to add.

---

edgraph/server.go, line 727 at r7 (raw file):

				continue
			}
			key, err := strconv.ParseInt(nq.Subject, 10, 64)

Why not use ParseUint?

---

edgraph/server.go, line 729 at r7 (raw file):

			key, err := strconv.ParseInt(nq.Subject, 10, 64)
			if err != nil {
				continue

Shouldn't errors be returned to the user instead of being silently ignored?

---

edgraph/server.go, line 744 at r7 (raw file):

			objectValue, err := types.ObjectValue(val.Tid, val.Value)
			if err != nil {
				// Conversion failed, deleting the element

Is this something that is expected or should be returned to the user?

[harshil-goel]

Reviewable status: 0 of 4 files reviewed, 16 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

chunker/rdf_state.go, line 139 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Add a comment here why just 'u` and 'v'?

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1629 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

It shouldn't delete the value in that case. Can we add a testcase for this as well? Basically store the variable as

me () {
      updated_amt as math(amt+1)

And then use this in the delete, there should not be any change in the result.

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1655 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Yeah that's a good one to add.

Done.

---

edgraph/server.go, line 706 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

query.Request should just be named req or something
And, also I think it makes sense to export the type of req.Vars because we are clearly using it outside the package where it is defined.

Done.

---

edgraph/server.go, line 727 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why not use ParseUint?

Done.

---

edgraph/server.go, line 729 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Shouldn't errors be returned to the user instead of being silently ignored?

We shouldn't stop executing the mutations due to failure in one NQuad. So we are just logging it for now.

---

edgraph/server.go, line 744 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this something that is expected or should be returned to the user?

We shouldn't stop executing the mutations due to failure in one NQuad. So we are just logging it for now.

[pawanrawal]

I am not happy with silently ignoring mutations instead of returning an error. Rest looks good.

Reviewed 1 of 3 files at r8.
Reviewable status: 1 of 4 files reviewed, 15 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, and @pullrequest[bot])

---

edgraph/server.go, line 729 at r7 (raw file):

Previously, harshil-goel wrote…

We shouldn't stop executing the mutations due to failure in one NQuad. So we are just logging it for now.

This seems misleading to me. The user would think that the mutation has gone through without errors. Users rarely check the logs to find out if there was an error in the request that they sent. I think we should be returning an error here. Happy to hear @mangalaman93's thoughts on this well.

---

edgraph/server.go, line 744 at r7 (raw file):

Previously, harshil-goel wrote…

We shouldn't stop executing the mutations due to failure in one NQuad. So we are just logging it for now.

Why shouldn't we stop executing the mutations in this case? This error would happen when there is a mismatch between the type of value and its type that is passed in Tid which would be a bug in our handling. There are more chances of the user finding it out if we return an error here rather than silently ignoring the mutation and logging it.

[mangalaman93]

Reviewable status: 1 of 4 files reviewed, 22 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/upsert_test.go, line 1445 at r8 (raw file):

	require.NoError(t, err)

	// User3 doesn't have all the fields. This test ensures

This comment looks misplaced.

---

dgraph/cmd/alpha/upsert_test.go, line 1591 at r8 (raw file):

	_, _, _, err = mutationWithTs(m5, "application/rdf", false, true, 0)
	require.NoError(t, err)

We should query after this to ensure that no amount value was modified.

---

dgraph/cmd/alpha/upsert_test.go, line 1592 at r8 (raw file):

	require.NoError(t, err)

	//Checking support for aggregate variable in upsert

Space after //

---

dgraph/cmd/alpha/upsert_test.go, line 1660 at r8 (raw file):

	// Checking Delete in Val
	m8 := `

Could you add the same test before the amount are set to the same value too?

---

dgraph/cmd/alpha/upsert_test.go, line 1684 at r8 (raw file):

	// Bulk Delete: delete everyone's branch
	m4 := `

Fix the sequence numbers. I see m4 after m8 and see whether it is possible to break this test into multiple different tests.

---

edgraph/server.go, line 724 at r4 (raw file):

Previously, harshil-goel wrote…

Done.

I thought this was unnecessary, just a comment would have been fine for now. We will be computing the length every time the function is called.

---

edgraph/server.go, line 729 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This seems misleading to me. The user would think that the mutation has gone through without errors. Users rarely check the logs to find out if there was an error in the request that they sent. I think we should be returning an error here. Happy to hear @mangalaman93's thoughts on this well.

Agree with Pawan.

---

edgraph/server.go, line 744 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why shouldn't we stop executing the mutations in this case? This error would happen when there is a mismatch between the type of value and its type that is passed in Tid which would be a bug in our handling. There are more chances of the user finding it out if we return an error here rather than silently ignoring the mutation and logging it.

Yeah, this one is a tricky case. This should happen because of a bug. We had a discussion with @manishrjain, though, I like that a user would come and raise an issue with us if we return an error but I don't like that they can't continue with the mutation here. Is there a way to achieve both?

---

edgraph/server.go, line 677 at r8 (raw file):

	vars := make(map[string]struct{})
	updateVars := func(s string) {
		if strings.HasPrefix(s, "uid(") || strings.HasPrefix(s, "val(") {

Add a test for capital VAL and UID with whatever is expected result.

---

edgraph/server.go, line 703 at r8 (raw file):

// updateValInMutations picks the val() from object and replaces it with its value
// Assumption is that Subject should contain UID, whereas Object should contain Val

can instead of should

---

edgraph/server.go, line 705 at r8 (raw file):

// Assumption is that Subject should contain UID, whereas Object should contain Val
// If val(variable) exists in a query, but the values are not there for the variable,
// it will delete the mutation itself, and ignore the mutation silently

it will delete the mutation itself is redundant.

---

edgraph/server.go, line 719 at r8 (raw file):

	}

	updateNQuads := func(nquads []*api.NQuad) []*api.NQuad {

We need to move out this function out of the outer function. Not sure why this was defined inside, are we using any variables from outer function?

Also, this function has too many if cases. Please refactor it to make it easy to read.

---

edgraph/server.go, line 720 at r8 (raw file):

	updateNQuads := func(nquads []*api.NQuad) []*api.NQuad {
		nquadsReplacement := nquads[:0]

Could just call newNQuads

---

edgraph/server.go, line 720 at r8 (raw file):

	updateNQuads := func(nquads []*api.NQuad) []*api.NQuad {
		nquadsReplacement := nquads[:0]

Are we guaranteed to always replace 1 NQuad with 1 or fewer NQuad?

---

edgraph/server.go, line 730 at r8 (raw file):

			if err != nil {
				// Conversion failed, ignoring the nquad. Ideally,
				// it shouldn't happen as it query return value.

as it query return value?

[pawanrawal]

Reviewable status: 1 of 4 files reviewed, 22 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

edgraph/server.go, line 744 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Yeah, this one is a tricky case. This should happen because of a bug. We had a discussion with @manishrjain, though, I like that a user would come and raise an issue with us if we return an error but I don't like that they can't continue with the mutation here. Is there a way to achieve both?

Well it could be achieved if we created GitHub issues automatically for some of these errors that are unexpected along with logging them. We also need to do something like this (creating a GitHub issue) for any panics that are encountered while executing a query or mutation but that's a separate thing.

[harshil-goel]

Reviewable status: 0 of 4 files reviewed, 22 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/upsert_test.go, line 1445 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This comment looks misplaced.

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1591 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

We should query after this to ensure that no amount value was modified.

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1592 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Space after //

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1660 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Could you add the same test before the amount are set to the same value too?

Done.

---

dgraph/cmd/alpha/upsert_test.go, line 1684 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Fix the sequence numbers. I see m4 after m8 and see whether it is possible to break this test into multiple different tests.

Done.

---

edgraph/server.go, line 724 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I thought this was unnecessary, just a comment would have been fine for now. We will be computing the length every time the function is called.

Done

---

edgraph/server.go, line 729 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Agree with Pawan.

For now, we have decided to not do it

---

edgraph/server.go, line 744 at r7 (raw file):

Well it could be achieved if we created GitHub issues automatically for some of these errors that are unexpected along with logging them. We also need to do something like this (creating a GitHub issue) for any panics that are encountered while executing a query or mutation but that's a separate thing.
For now, we have decided to not do it

---

edgraph/server.go, line 677 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Add a test for capital VAL and UID with whatever is expected result.

Done.

---

edgraph/server.go, line 703 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

can instead of should

Done.

---

edgraph/server.go, line 705 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

it will delete the mutation itself is redundant.

Done.

---

edgraph/server.go, line 719 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

We need to move out this function out of the outer function. Not sure why this was defined inside, are we using any variables from outer function?

Also, this function has too many if cases. Please refactor it to make it easy to read.

Done

---

edgraph/server.go, line 720 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Could just call newNQuads

Done.

---

edgraph/server.go, line 720 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Are we guaranteed to always replace 1 NQuad with 1 or fewer NQuad?

Yeah. We are just replacing val(variable_name) in place.

---

edgraph/server.go, line 730 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

as it query return value?

Done.

[manishrjain]

Reviewed 1 of 5 files at r1, 1 of 3 files at r8, 2 of 2 files at r9.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @golangcibot, @harshil-goel, @mangalaman93, @pawanrawal, and @pullrequest[bot])

---

edgraph/server.go, line 729 at r7 (raw file):

Previously, harshil-goel wrote…

For now, we have decided to not do it

Shouldn't happen anyways. Let's just log and ignore. Returning errors or doing check failures isn't a great experience.

---

edgraph/server.go, line 744 at r7 (raw file):

Previously, harshil-goel wrote…

Well it could be achieved if we created GitHub issues automatically for some of these errors that are unexpected along with logging them. We also need to do something like this (creating a GitHub issue) for any panics that are encountered while executing a query or mutation but that's a separate thing.
For now, we have decided to not do it

We should create a GitHub issue about catching the panics and sending them over to ping.dgraph.io or something. That'd be useful for us.

Regarding this silent ignore, if we can't convert stored type to schema type, there's no need to cancel the mutation. This is the current behavior for the query as well. Log it and just move on.

---

edgraph/server.go, line 750 at r9 (raw file):

		}

		key, err := strconv.ParseUint(nq.Subject, 10, 64)

nq.Subject, 0, 64