Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New upsert block #3412

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mangalaman93
Copy link
Contributor

commented May 14, 2019

Related to #3197
Fixes #3059

Example upsert operation -

   upsert {
      query {
        me(func: eq(email, "aman@dgraph.io")) {
          v as uid
        }
      }
      mutation {
          set {
            uid(v) <name> "Aman" .
            uid(v) <email> "aman@dgraph.io" .
          }
      }
    }

Probably in the next version -

  • Add support for multi valued variables
  • Support for val function
  • Add support for @if
  • Fuzz tests

Changes in other repos -

  • Add tests in dgo repo
  • Python, Java and JS client protobuf, test

Todo

  • Add lots of tests for RDF and JSON
  • Add documentation (docs.dgraph.io)
  • Add support for tracing
  • JSON format for upsert block
  • Add V(3) logging
  • Support for uid function
  • Fragments in upsert block
  • Two query blocks inside upsert block?
  • no query block inside upsert block?
  • no mutation block inside upsert block?
  • no variable in query block?
  • support for blank nodes in mutation block?
  • Ensure upsert only generates 1 UID

This change is Reviewable

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from 4d77d80 to 47fa5c0 May 20, 2019

Show resolved Hide resolved gql/state.go Outdated
Show resolved Hide resolved gql/state.go Outdated

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch 2 times, most recently from 45726c5 to 378caa0 May 20, 2019

Show resolved Hide resolved gql/parser_mutation.go Outdated

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from 378caa0 to 11b7e59 May 21, 2019

@mangalaman93 mangalaman93 changed the title [WIP] New Transaction API [WIP] New mutation using txn block May 21, 2019

Show resolved Hide resolved gql/state.go Outdated
Show resolved Hide resolved gql/state.go Outdated

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch 3 times, most recently from d936cba to 1ebe573 May 21, 2019

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch 2 times, most recently from 71f1110 to 62afd15 May 28, 2019

Show resolved Hide resolved gql/state.go Outdated
Show resolved Hide resolved gql/state.go Outdated
Show resolved Hide resolved gql/state.go Outdated

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch 3 times, most recently from c3a0744 to d4e5dde May 30, 2019

@mangalaman93 mangalaman93 changed the base branch from master to aman/acl_refactor May 31, 2019

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from d4e5dde to b138da7 May 31, 2019

@mangalaman93 mangalaman93 force-pushed the aman/acl_refactor branch from fee6274 to d153e45 May 31, 2019

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from b138da7 to 680c279 May 31, 2019

@mangalaman93 mangalaman93 force-pushed the aman/acl_refactor branch from d153e45 to 6f3c807 May 31, 2019

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from 680c279 to 2821a75 May 31, 2019

Show resolved Hide resolved edgraph/server.go Outdated

@mangalaman93 mangalaman93 changed the title [WIP] New mutation using txn block [WIP] New upsert block May 31, 2019

@mangalaman93 mangalaman93 reopened this Jun 1, 2019

@mangalaman93 mangalaman93 changed the base branch from aman/acl_refactor to master Jun 1, 2019

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch 2 times, most recently from a5d813c to 4fe3ff1 Jun 1, 2019

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

Add support for parsing functions in RDF (#3412)
Following are now possible (useful in upsert API)
  * uid(v) <predicate> "object" .
  * <0x01> <predicate> uid(foo) .

For now, we only support uid function. In future, we can
add support for more functions.

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from 74e9a0b to 15384a3 Jun 13, 2019

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

@mangalaman93
Copy link
Contributor Author

left a comment

Reviewable status: 4 of 15 files reviewed, 32 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, @martinmr, and @MichaelJCompton)


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

Previously, manishrjain (Manish R Jain) wrote…

Can you make a function out of this and use it in both the places?

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

This can be the first thing to check. But, I doubt it is of much usage, considering Mutate must have already checked it above and these are cheap ops.

Done.

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from 15384a3 to 52273f0 Jun 13, 2019

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

Add support for parsing functions in RDF (#3412)
Following are now possible (useful in upsert API)
  * uid(v) <predicate> "object" .
  * <0x01> <predicate> uid(foo) .

For now, we only support uid function. In future, we can
add support for more functions.

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

mangalaman93 added a commit that referenced this pull request Jun 13, 2019

mangalaman93 added some commits May 31, 2019

Add support for parsing functions in RDF (#3412)
Following are now possible (useful in upsert API)
  * uid(v) <predicate> "object" .
  * <0x01> <predicate> uid(foo) .

For now, we only support uid function. In future, we can
add support for more functions.

@mangalaman93 mangalaman93 force-pushed the aman/upsert branch from 52273f0 to 384455e Jun 13, 2019

@gitlw
Copy link
Contributor

left a comment

Reviewable status: 3 of 15 files reviewed, 44 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, @martinmr, and @MichaelJCompton)


chunker/rdf/parse.go, line 86 at r10 (raw file):

			}
			it.Next()
			if item = it.Item(); item.Typ != itemVarName {

Question: in the old code, it seems the varName is not stored into the rnq, do you know which part of the code actually make use of the var name? I'm just checking to make sure we are not breaking any existing feature. Thanks!


chunker/rdf/parse.go, line 205 at r10 (raw file):

}

func parseFunction(it *lex.ItemIterator) (string, error) {

Add doc to this function, preferable with an example


chunker/rdf/state.go, line 446 at r10 (raw file):

	l.IgnoreRun(isSpace)

	for {

This for loop can be replaced with the l.AcceptRun function


dgraph/cmd/alpha/upsert_test.go, line 225 at r10 (raw file):

	require.True(t, contains(preds, "age"))

	keys, preds, _, err = mutationWithTs(m1, "application/rdf", false, true, true, 0)

This block seems to be exactly the same with the block above. Should we remove the duplicate or do you intend to test something different?


dgraph/cmd/alpha/upsert_test.go, line 277 at r10 (raw file):

}`
	_, _, _, err := mutationWithTs(m1, "application/rdf", false, true, true, 0)
	require.Contains(t, err.Error(), "Some variables are used but not defined")

Using the name 42 as a variable name can be very confusing. It seems it's better to validate that the variable name does not start with a digit.


dgraph/cmd/alpha/upsert_test.go, line 310 at r10 (raw file):

}`
	_, _, _, err := mutationWithTs(m1, "application/rdf", false, true, true, 0)
	require.Contains(t, err.Error(), "Some variables are defined but not used")

Better to show which variables are not being used in the error.


dgraph/cmd/alpha/upsert_test.go, line 370 at r10 (raw file):

	require.Contains(t, res, "56")
	require.Contains(t, res, "true")

This test seems to have too much logic. I'd suggest breaking it apart into two tests with the logic above around "oldest" in one, and the logic about "friend" in another one.


dgraph/cmd/alpha/upsert_test.go, line 419 at r10 (raw file):

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

There is no query to validate the result of m3. Do you intend to add one?


dgraph/cmd/alpha/upsert_test.go, line 532 at r10 (raw file):

	require.Contains(t, res, "true")

	m2 := `

again I'd suggest breaking this test into two.


dgraph/cmd/alpha/upsert_test.go, line 641 at r10 (raw file):

	res, _, err := queryWithTs(q, "application/graphql+-", 0)
	require.NoError(t, err)
	require.Contains(t, res, "user1")

I find it confusing to make uid(u) and _:u represent two different things.
I think it's better to make sure there is no overlap between the vars inside the uid(...) blocks and the vars using _:var syntax.


dgraph/cmd/alpha/upsert_test.go, line 713 at r10 (raw file):

	require.Contains(t, res, "user1@dgraph.io")
	require.Contains(t, res, "user2@dgraph.io")
	require.True(t, len(strings.Split(res, "user")) == 6)

It's not immediately obvious how the result should have 6 counts of the user string.
Either remove this line or better use the z.CompareJson to make sure the result matches what we expect.


gql/parser_mutation.go, line 92 at r10 (raw file):

			}

		// upsert { ===>mutation<=== {...} query{...}}

It seems this comment and the comment on line 107 should be swapped.

@gitlw
Copy link
Contributor

left a comment

Other than the comments posted, the rest of the PR looks good. :)

Reviewable status: 3 of 15 files reviewed, 44 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, @martinmr, and @MichaelJCompton)

@martinmr
Copy link
Member

left a comment

Some minor comments. I am confused by the diff, however. I see a lot of parsing code. I thought that was already submitted in a separate PR.

Reviewed 12 of 16 files at r10.
Reviewable status: all files reviewed, 47 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, and @MichaelJCompton)


chunker/json/parse.go, line 148 at r10 (raw file):

		}

		// Handle the uid function in upsert block

minor: in the upsert block


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

Previously, mangalaman93 (Aman Mangal) wrote…

Will fix this in another commit/PR

add a TODO for now.


gql/parser.go, line 457 at r10 (raw file):

}

// ParseWithNeedVars performing parsing of a query with given needVars.

minor: s/performing/performs


gql/parser.go, line 476 at r10 (raw file):

// }
//
// we need to pass the variable name v through the needVars parameter. Otherwise, an error

"The variable name v needs to be passed through..."

@MichaelJCompton
Copy link
Contributor

left a comment

I want to understand something before I approve:

In the update case in your examples, what happens if the user doesn't exist? Won't this create a new node with just the age? Which is probably not what I wanted. So to use that, I'd have to know beforehand that "user@dgraph.io" exists ... which means I probably had to query to check ... which kinda defeats the purpose of the upsert block. Feels to me like what I want to do if the node already exists and what I want to do if it doesn't are two different things.

Does that make sense? Is it a concern? Or is that just a wrinkle that gets smoothed out by coming feature additions (like the if bits) to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.