[BREAKING] Support for fetching facets from value edge list #4267

ashish-goswami · 2019-11-13T08:59:48Z

Covers following things:

Support for fetching facets from value edge list
Response format change in case of facets to have uniform response format across, uid/value/list edges
Fix broken tests

This change is

golangcibot · 2019-11-13T09:01:47Z

query/outputnode.go

@@ -209,6 +210,38 @@ func (fj *fastJsonNode) writeKey(out *bytes.Buffer) error {
 	return nil
 }

+func attachFacets(fj *fastJsonNode, fieldName string, isList bool, fList []*api.Facet, facetIdx int) error {


line is 108 characters (from lll)

pawanrawal

Please also check with @campoy, but I think the idea was to put the change in response format behind a feature flag so as not to break backward compatibility for existing users.

Reviewed 1 of 5 files at r2, 3 of 4 files at r4, 4 of 4 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @mangalaman93, @manishrjain, and @martinmr)

posting/list.go, line 972 at r5 (raw file):

	var facets []*api.Facet
	err := l.iterate(readTs, 0, func(p *pb.Posting) error {
		if len(p.LangTag) == 0 {

Why do we skip these? The posting which belongs to a language can also have facets. Can you please check what happens in the case of expand(_all_) @facets where one of the expanded predicate is of scalar list type and has facets on language posting.

posting/list.go, line 978 at r5 (raw file):

	})

	return facets, err

I have a feeling that this might not work well in all scenarios. So we are collecting facets across different postings, each posting can have multiple facets. Like below.

"ashish" : []*api.Facet{}
"pawan": []*api.Facet{}

The number of facets belonging to each value(posting) can be different. If we put all of them in the same list, then how do we later know which posting do certain facets belong to?

posting/list.go, line 1164 at r5 (raw file):

	if listType {
		fs, err := l.AllUntaggedFacets(readTs)

Facets method already has a read lock and AllUntaggedFacets is also acquiring it again and it doesn't need to. AllUntaggedFacets should instead assert that a read lock is held and should instead be an internal function.

query/outputnode.go, line 222 at r5 (raw file):

	}

	for fName, facetList := range facetMap {

I'd need you to walk me through the changes in this file as I don't have a very good mental map of what is happening here.

query/query_facets_test.go, line 1631 at r5 (raw file):

	query := `{
		q(func: uid(0x1)) {
			name@en

Can we fetch the facets for this as well?

query/query_facets_test.go, line 1632 at r5 (raw file):

		q(func: uid(0x1)) {
			name@en
			alt_name @facets

Can you also add a case where alt_name has multiple values but not all the values have the same number of facets.

Also, please add a case where we only fetch a subset of values, like

alt_name @facets(origin)

martinmr

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @mangalaman93, @manishrjain, and @pawanrawal)

posting/list.go, line 972 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why do we skip these? The posting which belongs to a language can also have facets. Can you please check what happens in the case of expand(_all_) @facets where one of the expanded predicate is of scalar list type and has facets on language posting.

tag and language is the same term. So if the method is meant for retrieving only facets from untagged values, then it should skip values that have a language tag.

ashish-goswami

Reviewable status: 4 of 8 files reviewed, 7 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @golangcibot, @mangalaman93, @manishrjain, and @pawanrawal)

posting/list.go, line 978 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I have a feeling that this might not work well in all scenarios. So we are collecting facets across different postings, each posting can have multiple facets. Like below.
"ashish" : []*api.Facet{}
"pawan": []*api.Facet{}
The number of facets belonging to each value(posting) can be different. If we put all of them in the same list, then how do we later know which posting do certain facets belong to?

Thanks for pointing it out. We fixed it.

posting/list.go, line 1164 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Facets method already has a read lock and AllUntaggedFacets is also acquiring it again and it doesn't need to. AllUntaggedFacets should instead assert that a read lock is held and should instead be an internal function.

Done.

query/outputnode.go, line 213 at r4 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 108 characters (from lll)

Done.

query/query_facets_test.go, line 1631 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we fetch the facets for this as well?

Done.

query/query_facets_test.go, line 1632 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can you also add a case where alt_name has multiple values but not all the values have the same number of facets.

Also, please add a case where we only fetch a subset of values, like
alt_name @facets(origin)

Done.

pawanrawal

Reviewed 4 of 4 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @golangcibot, @mangalaman93, and @manishrjain)

posting/list.go, line 978 at r5 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Thanks for pointing it out. We fixed it.

This should still be an internal method allUntaggedFacets and should assert read lock is held.

posting/list.go, line 965 at r6 (raw file):

// AllUntaggedFacets returns all facets of all untagged values.
func (l *List) AllUntaggedFacets(readTs uint64) ([]*pb.Facets, error) {

Please add a comment that this is only called for predicates of type list.

ashish-goswami

Reviewable status: 4 of 8 files reviewed, 4 unresolved discussions (waiting on @animesh2049, @golangcibot, @mangalaman93, @manishrjain, and @pawanrawal)

posting/list.go, line 978 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This should still be an internal method allUntaggedFacets and should assert read lock is held.

Done.

posting/list.go, line 965 at r6 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Please add a comment that this is only called for predicates of type list.

Done.

query/outputnode.go, line 222 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I'd need you to walk me through the changes in this file as I don't have a very good mental map of what is happening here.

Discussed this offline.

manishrjain

Got a couple of comments. Prefix the title with a big [BREAKING], so we all know. Let's keep it in a branch and then merge in master for v1.2.

Reviewed 1 of 4 files at r4, 2 of 4 files at r5, 1 of 4 files at r6, 4 of 4 files at r7.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @golangcibot, @mangalaman93, and @pawanrawal)

posting/list.go, line 971 at r7 (raw file):

	err := l.iterate(readTs, 0, func(p *pb.Posting) error {
		if len(p.LangTag) == 0 {
			facets = append(facets, &pb.Facets{Facets: p.Facets})

facets = append(facets, p.Facets...)

Also, do whatever copying you need here.

posting/list.go, line 1169 at r7 (raw file):

		for _, fcts := range fs {
			fcs = append(fcs, &pb.Facets{Facets: facets.CopyFacets(fcts.Facets, param)})

Avoid copying here.

query/outputnode.go, line 213 at r4 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Done.

(fj *fastJsonNode) attachFacets(...)

robsws · 2019-11-29T14:24:30Z

Please let me know if I should post this elsewhere, but I've built this branch from source and I have an issue with the indexes in the facet data object. Here's an example - I'm running the following query:

{
  countries(func: type(Country)) {
    Name @facets(start_time, end_time) 
  }
}

That gives me the following results:

{
  "data": {
    "countries": [
      {
        "Name|end_time": {
          "0": "9999-12-31T00:00:00Z"
        },
        "Name|start_time": {
          "0": "1753-01-01T00:00:00Z"
        },
        "Name": [
          "Isle of Man"
        ]
      },
      {
        "Name|end_time": {
          "1": "9999-12-31T00:00:00Z"
        },
        "Name|start_time": {
          "1": "1753-01-01T00:00:00Z"
        },
        "Name": [
          "Chile"
        ]
      },
      {
        "Name|end_time": {
          "2": "9999-12-31T00:00:00Z"
        },
        "Name|start_time": {
          "2": "1753-01-01T00:00:00Z"
        },
        "Name": [
          "Moldova"
        ]
      },

      ...

      {
        "Name|end_time": {
          "250": "9999-12-31T00:00:00Z"
        },
        "Name|start_time": {
          "250": "1753-01-01T00:00:00Z"
        },
        "Name": [
          "Sahrawi"
        ]
      },
      {
        "Name|end_time": {
          "251": "9999-12-31T00:00:00Z"
        },
        "Name|start_time": {
          "251": "1753-01-01T00:00:00Z"
        },
        "Name": [
          "Serbia"
        ]
      }
    ]
  },

I was expecting that the keys for the "Name|end_time" and "Name|start_time" objects would refer back into the "Name" array, but it seems that they increment with each successive node in the result set. Was my assumption wrong here or is this not working as intended?

hypermodeinc/dgraph#4267 gets released

pawanrawal · 2019-12-02T06:30:46Z

@robsws that seems like a bug with the current implementation. Thanks for sharing this. @ashish-goswami could you please see that this is addressed before the PR is merged? We need some test cases which start with multiple uids at the root.

ashish-goswami · 2019-12-05T14:03:17Z

Hey @robsws, Thanks for pointing this. We have fixed it now. Please verify.

zhaolin-st · 2020-01-02T03:21:37Z

will this feature merge in the future version?

ashish-goswami · 2020-01-03T15:52:08Z

@zhaolin-st Yes this will be merged in future, most likely in v1.2.

martinmr

Hey, #4497 is fixed by this PR. But could you add an additional test to ensure facets in preds of list type work correctly with the expand function? The example in the issue should be helpful for that.

Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @golangcibot, @mangalaman93, @manishrjain, and @pawanrawal)

zhaolin-st · 2020-01-06T07:52:21Z

@zhaolin-st Yes this will be merged in future, most likely in v1.2.

@ashish-goswami
Thanks! This would solve my problems.
https://discuss.dgraph.io/t/how-can-i-query-facets-with-multi-property-better-facets-dgraph-in-v1-1-1/5724
and this one
https://discuss.dgraph.io/t/how-to-use-expand-sometype-if-some-predicate-of-the-type-are-list/5764/4

ashish-goswami

Reviewable status: 1 of 9 files reviewed, 6 unresolved discussions (waiting on @animesh2049, @ashish-goswami, @golangcibot, @mangalaman93, @manishrjain, and @pawanrawal)

query/outputnode.go, line 213 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

(fj *fastJsonNode) attachFacets(...)

done.

Related to hypermodeinc/dgraph@1a0f2c9 hypermodeinc/dgraph#4267

animesh2049 and others added 8 commits October 17, 2019 16:46

Handle list case

f4ef780

New output format for facets

dd0d9e9

Modify tests for new format

6e5e0f6

Merge remote-tracking branch 'origin/master' into animesh2049/issue_4081

47d9689

Add tests, fix issue when facet exists but uid node is empty

a2a0d35

Temp

7b809d0

Format test query responses

2b10be3

Fixed failing tests

79c6569

ashish-goswami requested review from martinmr, mangalaman93, pawanrawal and animesh2049 November 13, 2019 08:59

ashish-goswami requested review from manishrjain and a team as code owners November 13, 2019 08:59

golangcibot reviewed Nov 13, 2019

View reviewed changes

ashish-goswami added 3 commits November 13, 2019 15:06

Format facet's tests response

783f1db

Temp

5da3e11

Fix systest

42652dc

pawanrawal suggested changes Nov 14, 2019

View reviewed changes

martinmr reviewed Nov 15, 2019

View reviewed changes

ashish-goswami added 3 commits November 19, 2019 12:58

Address review comments

93f294f

Separate facets for value list

1f42077

Add test for fetching single facet in value list

8a989a6

ashish-goswami commented Nov 19, 2019

View reviewed changes

robsws mentioned this pull request Nov 22, 2019

@cascade disables value edge facet fetching in PR #4267 #4310

Closed

pawanrawal suggested changes Nov 26, 2019

View reviewed changes

ashish-goswami added 2 commits November 26, 2019 15:33

Merge remote-tracking branch 'origin/master' into animesh2049/issue_4081

e695b2c

Address review comments

71d9223

ashish-goswami commented Nov 26, 2019

View reviewed changes

manishrjain approved these changes Nov 28, 2019

View reviewed changes

pepoospina added a commit to uprtcl/js-uprtcl-server that referenced this pull request Nov 29, 2019

blocked by dgraph until

6df5a26

hypermodeinc/dgraph#4267 gets released

Fix indexing issue in value list facets

a1ed6fc

pepoospina mentioned this pull request Dec 11, 2019

Support fetching facets from value edge lists #4081

Closed

martinmr mentioned this pull request Jan 3, 2020

Cannot display list type data using expand function #4497

Closed

martinmr suggested changes Jan 3, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into animesh2049/issue_4081

1998abf

ashish-goswami changed the title ~~Support for fetching facets from value edge list~~ [breaking] Support for fetching facets from value edge list Jan 10, 2020

ashish-goswami changed the title ~~[breaking] Support for fetching facets from value edge list~~ [BREAKING] Support for fetching facets from value edge list Jan 10, 2020

Fixed failing tests

b538f9a

ashish-goswami commented Jan 10, 2020

View reviewed changes

ashish-goswami and others added 2 commits January 13, 2020 15:25

Add tests for expand

0170312

Merge branch 'master' into animesh2049/issue_4081

f0502e6

ashish-goswami merged commit 1a0f2c9 into master Jan 13, 2020

ashish-goswami deleted the animesh2049/issue_4081 branch January 13, 2020 12:21

emile-tawfik mentioned this pull request Jan 15, 2020

JSON mutation, Support facets for value edge lists #4581

Closed

mangalaman93 added a commit to hypermodeinc/dgo that referenced this pull request Jan 29, 2020

Fix facets test

ac0b49b

Related to hypermodeinc/dgraph@1a0f2c9 hypermodeinc/dgraph#4267

mangalaman93 mentioned this pull request Jan 29, 2020

Fix facets test hypermodeinc/dgo#115

Merged

mangalaman93 added a commit to hypermodeinc/dgo that referenced this pull request Jan 29, 2020

Fix facets test

e8b9d95

Related to hypermodeinc/dgraph@1a0f2c9 hypermodeinc/dgraph#4267

MichelDiz mentioned this pull request Mar 10, 2020

Make the new Facet response Optional. #4907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING] Support for fetching facets from value edge list #4267

[BREAKING] Support for fetching facets from value edge list #4267

ashish-goswami commented Nov 13, 2019 •

edited by manishrjain

Loading

golangcibot Nov 13, 2019

pawanrawal left a comment

martinmr left a comment

ashish-goswami left a comment

pawanrawal left a comment

ashish-goswami left a comment

manishrjain left a comment

robsws commented Nov 29, 2019

pawanrawal commented Dec 2, 2019

ashish-goswami commented Dec 5, 2019

zhaolin-st commented Jan 2, 2020 •

edited

Loading

ashish-goswami commented Jan 3, 2020

martinmr left a comment

zhaolin-st commented Jan 6, 2020 •

edited

Loading

ashish-goswami left a comment

[BREAKING] Support for fetching facets from value edge list #4267

[BREAKING] Support for fetching facets from value edge list #4267

Conversation

ashish-goswami commented Nov 13, 2019 • edited by manishrjain Loading

golangcibot Nov 13, 2019

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

robsws commented Nov 29, 2019

pawanrawal commented Dec 2, 2019

ashish-goswami commented Dec 5, 2019

zhaolin-st commented Jan 2, 2020 • edited Loading

ashish-goswami commented Jan 3, 2020

martinmr left a comment

Choose a reason for hiding this comment

zhaolin-st commented Jan 6, 2020 • edited Loading

ashish-goswami left a comment

Choose a reason for hiding this comment

ashish-goswami commented Nov 13, 2019 •

edited by manishrjain

Loading

zhaolin-st commented Jan 2, 2020 •

edited

Loading

zhaolin-st commented Jan 6, 2020 •

edited

Loading