Allow querying all lang values of a predicate. #2910

martinmr · 2019-01-17T01:50:51Z

This PR introduces the syntax pred@* which returns all the values of a
predicate which contain a language tag.

Fixes #2141

This change is

This PR introduces the syntax pred@* which returns all the values of a predicate which contain a language tag.

manishrjain

Nice change. I think can be simplified by setting ExpandAll for the right SubGraph.

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @martinmr and @manishrjain)

gql/parser.go, line 2120 at r1 (raw file):

	for ; item.Typ == itemName || item.Typ == itemPeriod; item = it.Item() {
		langs = append(langs, item.Val)
		if item.Val == string(star) {

Why not have star as an itemStar, like itemPeriod?

Also, no need to track seenStar. You can just iterate over the langs below, it should be cheap.

gql/parser.go, line 2142 at r1 (raw file):

	it.Prev()

	if len(langs) > 1 && seenStar {

for _, l := range langs { if l == star && len(langs) > 0 { return err } }

Consolidates the logic into one places, avoids a variable.

gql/parser.go, line 2143 at r1 (raw file):

	if len(langs) > 1 && seenStar {
		return nil, x.Errorf("If * is used, no other languages are allowed in the language list")

"...allowed in the language list. Found: %v", langs

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

					lang := pc.LangTags[idx].Lang[i]

					// Queries of the form pred@* should only return values with a lang tag.

Why not also return values with no lang tag? Don't think we need to be strict about it.

worker/task.go, line 371 at r1 (raw file):

			var vals []types.Val
			queryAllLangs := len(q.Langs) == 1 && q.Langs[0] == "*"
			if q.ExpandAll || queryAllLangs {

Instead of doing an OR everywhere q.ExpandAll is set, why not just set q.ExpandAll upfront for the attribute with the language?

SubGraph is done per predicate/attribute, so we should be able to just set ExpandAll=true upfront, and let it do its thing.

martinmr

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @manishrjain)

gql/parser.go, line 2120 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why not have star as an itemStar, like itemPeriod?

Also, no need to track seenStar. You can just iterate over the langs below, it should be cheap.

star is a rune. itemPeriod, etc. are lexed items. I think '*' in a language list should still be considered an itemName instead of being given it's own special type.

I've removed the seenStar logic.

gql/parser.go, line 2142 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

for _, l := range langs { if l == star && len(langs) > 0 { return err } }

Consolidates the logic into one places, avoids a variable.

Done.

gql/parser.go, line 2143 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

"...allowed in the language list. Found: %v", langs

Done.

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

Previously, manishrjain (Manish R Jain) wrote…

Why not also return values with no lang tag? Don't think we need to be strict about it.

Done.

worker/task.go, line 371 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Instead of doing an OR everywhere q.ExpandAll is set, why not just set q.ExpandAll upfront for the attribute with the language?

SubGraph is done per predicate/attribute, so we should be able to just set ExpandAll=true upfront, and let it do its thing.

Done.

martinmr · 2019-01-18T20:46:26Z

Ready for another round of review.

Addressed comments

manishrjain

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

worker/task.go, line 370 at r2 (raw file):

			var vals []types.Val
			if len(q.Langs) == 1 && q.Langs[0] == "*" {

I meant that you set the pc.Params.expandAll to true for the lang attribute SubGraph upfront, as we're creating the Subgraphs. So, you don't need to special casing and extra ORs between expandAll and get all langs.

…langs

martinmr

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

worker/task.go, line 370 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I meant that you set the pc.Params.expandAll to true for the lang attribute SubGraph upfront, as we're creating the Subgraphs. So, you don't need to special casing and extra ORs between expandAll and get all langs.

Done.

Addressed comments

manishrjain

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

query/query.go, line 484 at r3 (raw file):

			}
		} else {
			if pc.Params.Alias == "" && len(pc.Params.Langs) > 0 && !pc.Params.expandAll {

revert this and others in this file.

query/query.go, line 517 at r3 (raw file):

				}

				if (pc.Params.expandAll) && len(pc.LangTags[idx].Lang) != 0 {

Don't need the brackets.

query/query.go, line 1011 at r3 (raw file):

	if len(out.Langs) == 1 && out.Langs[0] == "*" {
		sg.Params.expandAll = true
		out.ExpandAll = true

sg.Langs = empty.

martinmr

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

query/query.go, line 484 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

revert this and others in this file.

Done.

query/query.go, line 517 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't need the brackets.

Done.

query/query.go, line 1011 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

sg.Langs = empty.

Done.

manishrjain

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)

query/query0_test.go, line 134 at r4 (raw file):

	js := processToFastJsonNoErr(t, query)
	require.JSONEq(t,
		`{"data":{"people": [{"name@en":"Amit", "name@hi":"अमित", "name":""}]}}`,

why the empty name output? That seems like a side-effect.

martinmr

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

query/query0_test.go, line 134 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

why the empty name output? That seems like a side-effect.

The default name was set to empty https://github.com/dgraph-io/dgraph/blob/master/query/common_test.go#L665

Not a side effect.

Answered question

manishrjain

Thanks for working through with this. Really nice change!

Reviewed 1 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

This PR introduces the syntax pred@* which returns all the values of a predicate which contain a language tag.

Allow querying all lang values of a predicate.

30ae4d9

This PR introduces the syntax pred@* which returns all the values of a predicate which contain a language tag.

martinmr requested a review from manishrjain January 17, 2019 01:50

manishrjain previously requested changes Jan 17, 2019

View reviewed changes

martinmr commented Jan 17, 2019

View reviewed changes

Address review comments

bee1581

manishrjain previously requested changes Jan 19, 2019

View reviewed changes

martinmr added 2 commits January 18, 2019 18:25

Merge remote-tracking branch 'origin/master' into martinmr/query-all-…

9cd2c1e

…langs

Set ExpandAll in query.go

44ba0de

martinmr commented Jan 19, 2019

View reviewed changes

manishrjain suggested changes Jan 21, 2019

View reviewed changes

martinmr commented Jan 21, 2019

View reviewed changes

Address review comments.

e5543d9

manishrjain previously requested changes Jan 21, 2019

View reviewed changes

martinmr commented Jan 21, 2019

View reviewed changes

manishrjain approved these changes Jan 21, 2019

View reviewed changes

martinmr merged commit 3b08b15 into master Jan 21, 2019

martinmr deleted the martinmr/query-all-langs branch January 21, 2019 21:46

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019

Allow querying all lang values of a predicate. (hypermodeinc#2910)

e06d3e4

This PR introduces the syntax pred@* which returns all the values of a predicate which contain a language tag.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow querying all lang values of a predicate. #2910

Allow querying all lang values of a predicate. #2910

martinmr commented Jan 17, 2019 •

edited by manishrjain

Loading

manishrjain left a comment

martinmr left a comment

martinmr commented Jan 18, 2019

manishrjain left a comment

martinmr left a comment

manishrjain left a comment

martinmr left a comment

manishrjain left a comment

martinmr left a comment

manishrjain left a comment

Allow querying all lang values of a predicate. #2910

Allow querying all lang values of a predicate. #2910

Conversation

martinmr commented Jan 17, 2019 • edited by manishrjain Loading

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

martinmr commented Jan 18, 2019

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

martinmr commented Jan 17, 2019 •

edited by manishrjain

Loading