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

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

Merged
merged 21 commits into from Jan 13, 2020

Conversation

@ashish-goswami
Copy link
Member

ashish-goswami commented Nov 13, 2019

Fixes #4081

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 Reviewable

@ashish-goswami ashish-goswami requested review from manishrjain and dgraph-io/team as code owners Nov 13, 2019
@@ -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 {

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 13, 2019

line is 108 characters (from lll)

Copy link
Member

pawanrawal left a comment

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)
Copy link
Member

martinmr left a comment

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.

Copy link
Member Author

ashish-goswami left a comment

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.

Copy link
Member

pawanrawal left a comment

:lgtm:

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.

Copy link
Member Author

ashish-goswami left a comment

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.

Copy link
Member

manishrjain left a comment

:lgtm: 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

This comment has been minimized.

Copy link

robsws commented Nov 29, 2019

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?

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

This comment has been minimized.

Copy link
Member

pawanrawal commented Dec 2, 2019

@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

This comment has been minimized.

Copy link
Member Author

ashish-goswami commented Dec 5, 2019

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

@zhaolin-st

This comment has been minimized.

Copy link

zhaolin-st commented Jan 2, 2020

will this feature merge in the future version?

@ashish-goswami

This comment has been minimized.

Copy link
Member Author

ashish-goswami commented Jan 3, 2020

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

Copy link
Member

martinmr left a comment

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

This comment has been minimized.

@ashish-goswami 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 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
Copy link
Member Author

ashish-goswami left a comment

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.

ashish-goswami and others added 2 commits Jan 13, 2020
@ashish-goswami ashish-goswami merged commit 1a0f2c9 into master Jan 13, 2020
6 of 8 checks passed
6 of 8 checks passed
DeepSource: Go Analysis Failed: Issues detected that need attention.
Details
code-review/reviewable 8 files, 6 discussions left (animesh2049, ashish-goswami, golangcibot, mangalaman93, manishrjain, pawanrawal)
Details
Blockade (dgraph) TeamCity build finished
Details
CI (dgraph) TeamCity build finished
Details
Docs-Preview (dgraph) TeamCity build finished
Details
Docs-Preview/dgraph-f0502e6d89-39153 Deployment ready!
Details
GolangCI No issues found!
Details
license/cla Contributor License Agreement is signed.
Details
@ashish-goswami ashish-goswami deleted the animesh2049/issue_4081 branch Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.