Search code, repositories, users, issues, pull requests...

	}()

	needVars := findVars(gmu)

Looks like findVars can be called directly by doQueryInUpsert.

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

			}

			if strings.HasPrefix(nq.ObjectId, "uid(") {

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

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

		varsList = append(varsList, v)
	}
	glog.V(3).Infof("Variables used in mutation block: %v", varsList)

if glog.V(3) { glog.Infof(...) } would be faster.

// doQueryInUpsert processes a query in the upsert block.
func doQueryInUpsert(ctx context.Context, l *query.Latency, queryText string,
	needVars []string, startTs uint64) (map[string]string, error) {

I have a rule of thumb that 3 args and 2 return values are the best. 4 args and 3 return vals are pushing it. Anything beyond is not allowed.

Find a way to pass a struct around if that better encapsulates what you need.

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

	}

	if ctx.Err() != nil {

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.

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

		return nil, ctx.Err()
	}
	x.AssertTruef(startTs != 0, "Transaction timestamp is zero")

This function has an error return. So, better to just return error instead of crashing the server.

dgraph/cmd/alpha/upsert_test.go, line 27 at r9 (raw file):

		} else if len(v.Uids.Uids) == 1 {
			varToUID[name] = fmt.Sprintf("%d", v.Uids.Uids[0])
		}

You can also handle the case where == 0. And set the varToUID to a blank node here. Then you don't need to do repeat similar logic above.

gql/parser_mutation.go, line 68 at r9 (raw file):

	var queryFound bool

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

Think you're missing an equal sign on the left.

gql/parser_mutation.go, line 104 at r9 (raw file):

				}
				queryFound = true
				x.AssertTrue(it.Item().Typ == itemUpsertBlockOpContent)

Return error, don't assert.

gql/parser_mutation.go, line 112 at r9 (raw file):

				}
			} else if item.Val == "fragment" {
				x.AssertTrue(it.Item().Typ == itemUpsertBlockOpContent)

Return error, don't assert.

gql/state.go, line 357 at r9 (raw file):

	// TODO(Aman): we currently only have upsert block,
	// this won't work if we have other blocks too.
	if l.BlockDepth != 0 {

Can you add a comment about why if block depth is not zero, we assume that this is an upsert block?

gql/upsert_test.go, line 1 at r9 (raw file):

package gql

License.

mangalaman93

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

chunker/rdf/parse_test.go, line 942 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can you also add some tests where the input causes an error? Like when the number of brackets isn't right. Or something like this:

uid ( val <lives> "vrindavan" ) .

Done.

chunker/rdf/state.go, line 430 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

s/var/uid

Done.

Previously, manishrjain (Manish R Jain) wrote…

Is this the same as:
func contains(ps ...) bool {
}

Done

dgraph/cmd/alpha/upsert_test.go, line 46 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Ashish

But, I realize that you're trying to set the wrong name. Can you just call it "wrong" or something to make it clear for future readers, instead of introducing a spelling mistake.

Done.

dgraph/cmd/alpha/upsert_test.go, line 72 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I thought we were going to switch it to graphql+-, using the actual symbols.

Done.

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

Previously, manishrjain (Manish R Jain) wrote…

You can remove the uid(..) enclosure and put the blank node at the variable name. So, from uid(x) -> _:x. It would look nicer and would be easier for the end user to parse.

I replace uid(x) -> _:uid(x). I don't want to use _:x because x can potentially be used in the mutation block already. Blank nodes are still okay in an upsert.

Example -
set {
uid(x) "India" .
uid(x) "A" .
_:x "Manish" .
_:x "USA" .
}

I will add this as a test

Previously, manishrjain (Manish R Jain) wrote…

Looks like findVars can be called directly by doQueryInUpsert.

I am thinking of refactoring this in another commit/PR. Can't do in this one, doQueryInUpsert doesn't have access to gmu. The current flow is a bit strange, sometimes we use mu and sometimes gmu, the parsed version!

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

Previously, manishrjain (Manish R Jain) wrote…

if glog.V(3) { glog.Infof(...) } would be faster.

Done.

Previously, manishrjain (Manish R Jain) wrote…

I have a rule of thumb that 3 args and 2 return values are the best. 4 args and 3 return vals are pushing it. Anything beyond is not allowed.

Find a way to pass a struct around if that better encapsulates what you need.

Will fix this in another commit/PR

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

Previously, manishrjain (Manish R Jain) wrote…

This function has an error return. So, better to just return error instead of crashing the server.

Done.

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

Previously, manishrjain (Manish R Jain) wrote…

You can also handle the case where == 0. And set the varToUID to a blank node here. Then you don't need to do repeat similar logic above.

We don't have access to gmu here

gql/parser_mutation.go, line 28 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I named them here because I will have to declare these variables in the next line otherwise. Will that be better?

Done.

gql/parser_mutation.go, line 68 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Think you're missing an equal sign on the left.

Done.

gql/parser_mutation.go, line 104 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Return error, don't assert.

Done.

gql/parser_mutation.go, line 112 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Return error, don't assert.

Done.

gql/state.go, line 357 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can you add a comment about why if block depth is not zero, we assume that this is an upsert block?

Done.

gql/upsert_test.go, line 1 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

License.

Done.

mangalaman93

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.

gitlw

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

	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

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

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

dgraph/cmd/alpha/upsert_test.go, line 225 at r10 (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

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?

mangalaman93

@martinmr didn't merge that PR, though addressed all the comments. We will merge this PR instead.

@MichaelJCompton Not sure I completely follow. I am wondering what would you have liked in the update case if the user doesn't exist. The second case is an update case, because user already exists (otherwise, it would be called insert use case). In general, you do not really care whether user exists. You pretty much run the same query, irrespective of whether user exists. The Upsert block will create the user if it doesn't exist and update it otherwise.

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

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

Previously, martinmr (Martin Martinez Rivera) wrote…

minor: in the upsert block

This is just a comment.

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

Previously, gitlw (Lucas Wang) wrote…

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!

This code wasn't used before. This case will only occur when uid function is used in RDFs. I do not think we allowed that until now.

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

Previously, gitlw (Lucas Wang) wrote…

Add doc to this function, preferable with an example

Done.

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

Previously, gitlw (Lucas Wang) wrote…

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

Done.

Previously, gitlw (Lucas Wang) wrote…

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?

Added a comment, wanted to make sure that two runs work fine.

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

Previously, gitlw (Lucas Wang) wrote…

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.

Added a TODO.

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

Previously, gitlw (Lucas Wang) wrote…

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

Done.

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

Previously, gitlw (Lucas Wang) wrote…

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.

Done

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

Previously, gitlw (Lucas Wang) wrote…

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

Done.

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

Previously, gitlw (Lucas Wang) wrote…

again I'd suggest breaking this test into two.

Done.

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

Previously, gitlw (Lucas Wang) wrote…

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.

This restriction seems unnecessary as of now. This test just ensures that it is allowed, users shouldn't have such mutations in the first place if they find it confusing.

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

Previously, gitlw (Lucas Wang) wrote…

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.

Done.

Previously, martinmr (Martin Martinez Rivera) wrote…

add a TODO for now.

Done.

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

Previously, martinmr (Martin Martinez Rivera) wrote…

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

Done.

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

Previously, gitlw (Lucas Wang) wrote…

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

Done.

gitlw

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

manishrjain

Got some comments. Please address those before merging.

Note that my main concern with this PR is how many TODOs it introduces, because it relies on a refactoring happening in the future. So, please DO get back and do the refactoring ASAP.

Otherwise, nice change. This is a feature that the community has been craving for!

Reviewed 3 of 16 files at r10, 3 of 7 files at r11, 7 of 7 files at r13.
Reviewable status: all files reviewed, 35 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, and @martinmr)

chunker/rdf/state.go, line 450 at r13 (raw file):

	// before they can be used here. We throw an error if number of used variables
	// are different than number of defined variables.
	acceptVar := func(r rune) bool { return !(isSpace(r) || r == ')') }

This acceptVar isn't clear. It returns false if it finds a space or a right bracket.

Previously, mangalaman93 (Aman Mangal) wrote…

I am thinking of refactoring this in another commit/PR. Can't do in this one, doQueryInUpsert doesn't have access to gmu. The current flow is a bit strange, sometimes we use mu and sometimes gmu, the parsed version!

Add a TODO for any future cleanup that you're not tackling here.

Previously, mangalaman93 (Aman Mangal) wrote…

We don't have access to gmu here

Add a TODO in that case.

edgraph/server.go, line 637 at r13 (raw file):

// doQueryInUpsert processes a query in the upsert block.
// TODO(Aman): refactor this function along with doMutate

Refactor this ... to achieve X. That X must be mentioned. Otherwise, you wouldn't know what to refactor it for later.

gql/state.go, line 355 at r13 (raw file):

func lexTopLevel(l *lex.Lexer) lex.StateFn {
	// TODO(Aman): This won't work if more nested blocks other than the Upsert block

This TODO doesn't match any action item.

TODO: Do X to achieve Y because of Z.

mangalaman93

Reviewable status: all files reviewed, 35 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, and @martinmr)

chunker/rdf/state.go, line 450 at r13 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This acceptVar isn't clear. It returns false if it finds a space or a right bracket.

Because as soon as we find a space or a right bracket, we assume that we are done reading the variable. I can add more comments here.

Previously, manishrjain (Manish R Jain) wrote…

Add a TODO for any future cleanup that you're not tackling here.

I will raise a PR for this right away.