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

Use type when available to resolve expand predicates. #3214

Merged
merged 7 commits into from
Apr 4, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Mar 27, 2019

When a node has a dgraph.type edge, expandSubgraph will use the type
definition to determine the list of predicates to expand instead of
_predicate_.


This change is Reviewable

When a node has a dgraph.type edge, expandSubgraph will use the type
definition to determine the list of predicates to expand instead of
_predicate_.
@martinmr martinmr requested a review from a team March 27, 2019 00:43
Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr)


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

		<200> <automaker> "Ford" .
		<200> <make> "Focus" .

This is not important but I think most people would refer to "Ford" as the "make" and "Focus" as the "model".

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @codexnull)


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

Previously, codexnull (Javier Alvarado) wrote…

This is not important but I think most people would refer to "Ford" as the "make" and "Focus" as the "model".

Done.

@martinmr martinmr requested a review from a team April 2, 2019 23:56
Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Bunch of comments. Get final approval from @gitlw

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @martinmr)


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

			span.Annotate(nil, "expand(_reverse_)")
			if len(types) > 0 {
				typePreds := getNodePredicatesFromTypes(types)

preds := getPredicatesFromTypes(types)


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

				typePreds := getNodePredicatesFromTypes(types)

				rpreds, err := getReversePredicatesFromType(ctx, typePreds)

rpreds, err := getReversePredicates(ctx, preds)


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

	return result.ValueMatrix, nil
}

Add a TODO.


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

	return uniqueValues(result.ValueMatrix), nil
}

Add comments.


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

		typeDef, ok := schema.State().GetType(typeName)
		if !ok {
			return preds

continue?


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

}

func getReversePredicatesFromType(ctx context.Context, typePreds []string) ([]string, error) {

preds


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

	}

	schs, err := worker.GetSchemaOverNetwork(ctx, &pb.SchemaRequest{})

Maybe can be more specific about which predicates you want to get the schema for.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @manishrjain)


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

Previously, manishrjain (Manish R Jain) wrote…

preds := getPredicatesFromTypes(types)

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

rpreds, err := getReversePredicates(ctx, preds)

function name is already being used. I'll rename it in the next PR to minimize merge conflicts with the PR I already started.


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

Previously, manishrjain (Manish R Jain) wrote…

Add a TODO.

I already started the next PR and removed this function so I'll skip this to minimize merge conflicts.


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

Previously, manishrjain (Manish R Jain) wrote…

Add comments.

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

continue?

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

preds

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

Maybe can be more specific about which predicates you want to get the schema for.

Done.

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @codexnull and @manishrjain)

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @codexnull and @manishrjain)

@martinmr martinmr merged commit bcc2733 into master Apr 4, 2019
@martinmr martinmr deleted the martinmr/expand-using-types branch April 4, 2019 23:23
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
When a node has a dgraph.type edge, expandSubgraph will use the type
definition to determine the list of predicates to expand instead of
_predicate_.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants