Add support for conditional mutation in Upsert Block #3612

mangalaman93 · 2019-06-28T09:16:21Z

Fixes #3059
Related to #3412

Todo -

Lexing and Parsing
Execution support
Add support for multi valued variables
Support for JSON API
Support for val function
❌ Support for multiple mutation in single upsert block
Fuzz tests
Update docs

Changes in other repos -

Add tests in dgo repo
Python Client
Java Client
JS Client

This change is

pullrequest

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

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

pullrequest

This code looks great on a whole and well tested. Left a few comments / potential issues found inline.

Reviewed with ❤️ by PullRequest

edgraph/server.go

dgraph/cmd/alpha/http.go

gql/upsert_test.go

gql/state.go

edgraph/server.go

pawanrawal

Mostly looks good. Though needs comments explaining the implementation for conditional upsert in edgraph/server.go.

Reviewed 1 of 8 files at r1.
Reviewable status: 1 of 8 files reviewed, 11 unresolved discussions (waiting on @mangalaman93 and @manishrjain)

edgraph/server.go, line 574 at r2 (raw file):

	*query.Latency, error) {

	isThisUpsert := mu.Query != ""

Not sure if we need This in the names.
isUpsert and isCondUpsert seem verbose enough.

P.S. - This reminds of JS.

edgraph/server.go, line 575 at r2 (raw file):

	isThisUpsert := mu.Query != ""
	isThisCondUpsert := strings.TrimSpace(mu.Cond) != ""

This trimming should ideally be done in the parser.

edgraph/server.go, line 582 at r2 (raw file):

	}

	queryWithIf := mu.Query

This name i a bit misleading as this would only be queryWithIf in case of conditional upsert. Perhaps just have it as query?

edgraph/server.go, line 591 at r2 (raw file):

		// find out whether the if condition is true or false.
		queryWithIf = strings.TrimSuffix(strings.TrimSpace(mu.Query), "}") +
			` var(func: uid(0x01)) ` + cond + ` { __uid__ as uid }` + `}`

You could have the query as

varname as var(func: uid(0x01)) ` + cond

Thought what happens if user also defined __uid__ in some other query block. Do we allow variable names to start with __?

edgraph/server.go, line 591 at r2 (raw file):

		// find out whether the if condition is true or false.
		queryWithIf = strings.TrimSuffix(strings.TrimSpace(mu.Query), "}") +
			` var(func: uid(0x01)) ` + cond + ` { __uid__ as uid }` + `}`

Could you add comments on the idea about using 0x01 as a dummy ID here?

edgraph/server.go, line 714 at r2 (raw file):

			for _, o := range newObs {
				// Blank node has no meaning in case of deletion.
				if strings.HasPrefix(s, "_:uid(") ||

Why is it testing for the prefix to be _:uid, shouldn't it just check for _:?

pawanrawal

Just needs more tests to test out various scenarios and make sure we support all the use cases mentioned in the original issue.

Reviewable status: 1 of 8 files reviewed, 5 unresolved discussions (waiting on @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

edgraph/server.go, line 591 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This could be a problem, will add a todo. and what's the benefit of doing varname as var(...?

Its just more terse that way and achieves the same result.

mangalaman93

Added more test cases, will add more error test cases as well.

Reviewable status: 1 of 8 files reviewed, 5 unresolved discussions (waiting on @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

edgraph/server.go, line 591 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Its just more terse that way and achieves the same result.

Done.

pullrequest

	1 Message
⚠️	Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

gitlw

Reviewed 2 of 4 files at r3.
Reviewable status: 2 of 8 files reviewed, 11 unresolved discussions (waiting on @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

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

	*query.Latency, error) {

	isUpsert := mu.Query != ""

I feel the variable isUpsert is not needed, and simplifying it to
if mu.Query == "" {
return l, nil
}

is more intuitive.

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

	if isCondUpsert {
		// currently we do not have support for @if directly.
		cond := strings.Replace(mu.Cond, "@if", "@filter", 1)

Maybe the comment should say that @if and @filter are the same (at least for now)?

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

		// remove the trailing closing brace from the query
		upsertQuery = strings.TrimSuffix(strings.TrimSpace(mu.Query), "}")

Why is there an extra } at the end of the Query?

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

		// TODO(Aman): if the query already contains ___uid___ variable
		// this could lead to unexpected results.
		upsertQuery += `___uid___ as var(func: uid(0)) ` + cond + `}`

Explain that when cond is true, this will result in one Uid, and when cond is false, this will result in zero Uids.

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

	gmuDel := make([]*api.NQuad, 0, len(gmu.Del))
	for _, nq := range gmu.Del {
		newSubs := getNewVals(nq.Subject)

Add a comment above to say that if the Subject or Object is a variable, it can be transformed into multiple uids.

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

	// Update the values in mutation block from the query block.
	gmuSet := make([]*api.NQuad, 0, len(gmu.Set))
	for _, nq := range gmu.Set {

The logic is very similar to the block above.
We can extract a function to be used for both the gmu.Del, and gmu.Set, which can take an argument to specify whether the blank node entries should be included.

pawanrawal

Nice work on the test cases. Would be good to move some of them to parser_test.go and change the name of some others to make it obvious what they are testing.

Reviewable status: 2 of 8 files reviewed, 17 unresolved discussions (waiting on @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

dgraph/cmd/alpha/upsert_test.go, line 798 at r3 (raw file):

}

func TestConditionalUpsertExample0(t *testing.T) {

TestConditionalUpsert_UpsertNonExistentUser

dgraph/cmd/alpha/upsert_test.go, line 836 at r3 (raw file):

	require.NoError(t, err)
	require.Contains(t, res, "Wrong")

How about we try the mutation again with eq(len(v), 0) and name Ashish at this stage. That should be a NOOP.

dgraph/cmd/alpha/upsert_test.go, line 1045 at r3 (raw file):

}

func TestUpsertMultiValueEdge(t *testing.T) {

Could also add another test case where if say lt(len(c1), 3), then link add a new employee using blank nodes with company as company1 and link them to employees working for c1.

dgraph/cmd/alpha/upsert_test.go, line 1069 at r3 (raw file):

	q1 := `
{
  q(func: eq(works_for, "%s")) {

How about checking for both company1 and company2 here as eq can support multiple arguments to check connections in both directions?

dgraph/cmd/alpha/upsert_test.go, line 1115 at r3 (raw file):

}

func TestConditionalUpsertWithFilterErr(t *testing.T) {

This test possibly belongs in parser_test.go

dgraph/cmd/alpha/upsert_test.go, line 1138 at r3 (raw file):

}

func TestConditionalUpsertMissingAtErr(t *testing.T) {

This and some other tests below belong to parser_test.go

edgraph/server.go, line 591 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Done.

Also is cheaper to execute as we don't need to create the child SubGraph for uid and execute it.

mangalaman93

Reviewable status: 2 of 8 files reviewed, 17 unresolved discussions (waiting on @filter, @gitlw, @if, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])

dgraph/cmd/alpha/upsert_test.go, line 798 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

TestConditionalUpsert_UpsertNonExistentUser

This example does more than just that.

dgraph/cmd/alpha/upsert_test.go, line 836 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

How about we try the mutation again with eq(len(v), 0) and name Ashish at this stage. That should be a NOOP.

Done.

dgraph/cmd/alpha/upsert_test.go, line 1045 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Could also add another test case where if say lt(len(c1), 3), then link add a new employee using blank nodes with company as company1 and link them to employees working for c1.

Done.

dgraph/cmd/alpha/upsert_test.go, line 1069 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

How about checking for both company1 and company2 here as eq can support multiple arguments to check connections in both directions?

This query is used in more places than just here.

dgraph/cmd/alpha/upsert_test.go, line 1115 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This test possibly belongs in parser_test.go

The parsing of the if directive is done only during execution of the query. Can't put this test in parser_test.go

dgraph/cmd/alpha/upsert_test.go, line 1138 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This and some other tests below belong to parser_test.go

See above

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

Previously, gitlw (Lucas Wang) wrote…

I feel the variable isUpsert is not needed, and simplifying it to
if mu.Query == "" {
return l, nil
}

is more intuitive.

Done.

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

Previously, gitlw (Lucas Wang) wrote…

Maybe the comment should say that @if and @filter are the same (at least for now)?

Done.

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

Previously, gitlw (Lucas Wang) wrote…

Why is there an extra } at the end of the Query?

Added more comments for explanation.

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

Previously, gitlw (Lucas Wang) wrote…

Explain that when cond is true, this will result in one Uid, and when cond is false, this will result in zero Uids.

Done.

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

Previously, gitlw (Lucas Wang) wrote…

Add a comment above to say that if the Subject or Object is a variable, it can be transformed into multiple uids.

Done.

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

Previously, gitlw (Lucas Wang) wrote…

The logic is very similar to the block above.
We can extract a function to be used for both the gmu.Del, and gmu.Set, which can take an argument to specify whether the blank node entries should be included.

I thought it wasn't too much code. Extracting it out could make it hard to read.

manishrjain

Got a few comments.

Reviewed 1 of 8 files at r1, 3 of 6 files at r2, 2 of 4 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @filter, @gitlw, @if, @mangalaman93, @pawanrawal, and @pullrequest[bot])

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

    set {
      uid(v) <name> "Wrong" .
      uid(v) <email> "[email protected]" .

Don't use real email ids.

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

  }

  query {

Move the query above the mutation in all test cases.

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

  query {
    me(func: eq(email, "[email protected]")) {

Don't put real email ids here.

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

		//      * have 1 UID (the 0 UID) if the condition is false
		//
		// TODO(Aman): if the query already contains ___uid___ variable

Can instead use _dgraph-0xrandomnumber_.

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

	// If a variable doesn't have any UID, we generate one ourselves later.
	varToUID := make(map[string][]string)

Doesn't need to be string. Most likely should be []uint64.

Or, better, just leave it as the *pb.List?

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

		}

		if len(v.Uids.Uids) > 0 {

Don't need this if here.

gql/parser_mutation.go, line 130 at r4 (raw file):

			if mu, err = parseMutationBlock(it); err != nil {
				return nil, err
			}

mu.Cond = condText?

gql/state.go, line 94 at r4 (raw file):

// lexNameBlock lexes the blocks, for now, only upsert block
func lexNameBlock(l *lex.Lexer) lex.StateFn {
	for {

Looks like this for loop can now be removed.

gql/state.go, line 137 at r4 (raw file):

// lexNameUpsertOp parses the operation names inside upsert block
func lexNameUpsertOp(l *lex.Lexer) lex.StateFn {
	for {

This for loop can now be removed.

mangalaman93

Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @filter, @gitlw, @if, @manishrjain, @pawanrawal, and @pullrequest[bot])

dgraph/cmd/alpha/http.go, line 329 at r1 (raw file):

Previously, pullrequest[bot] wrote…

I may be over-optimizing code reuse here but this check looks very similar to the previous one for query could potentially range over the options if there might be more in the future as well. For example something like this:
for _, queryKey := range []string{"query", "cond"} {
    if queryText, ok := ms[queryKey]; ok && queryText != nil {
        mu.Query, err = strconv.Unquote(string(queryText.bs))
        if err != nil {
            x.SetStatus(w, x.ErrorInvalidRequest, err.Error())
            return
        }
    }
}

Done.

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