From d260053caaca8e55d1b98f5b9eca754b46357814 Mon Sep 17 00:00:00 2001 From: Ashish Goswami Date: Thu, 9 Jul 2020 18:36:23 +0530 Subject: [PATCH] (fix/dgraph): Fix facets response with normalize (#5690) Fixes: #5241 Fixes: DGRAPH-1670 This PR fixes issue with facets when it is retrieved in a query containing @normalize directive. While forming @normalize response, we flatten a fastJsonNode and make its grand children, direct children of it. This should be valid in all of cases except when fastJsonNode is parent of facets nodes. For example consider below data: <0x1> "Alice" . <0x1> "Bob" (from="college") . <0x1> "Roman" (from="school") . Also consider below query: q(func: uid(0x1)) @normalize { name: name friend: friends @facets } Expected response is: { "data": { "q": [ { "name": "Alice", "friends|from": { "0": "college", "1": "school" }, "friends": [ "Bob", "Roman" ] } ] } } But actual response is: { "data": { "q": [ { "0": "college", "1": "school", "friends": [ "Bob", "Roman" ], "name": "Alice" } ] } } Its happening because we are flattening facet parent node friends|from as well which have node "0" and "1" as children. We are solving it by having extra information in the node if it is a facets parent. (cherry picked from commit f4c28b8998aab054f53c6c3e0b7cff84a4e407a3) --- query/outputnode.go | 53 +++++++---- query/query_facets_test.go | 186 +++++++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 17 deletions(-) diff --git a/query/outputnode.go b/query/outputnode.go index 77dd3755c97..143fe0b2221 100644 --- a/query/outputnode.go +++ b/query/outputnode.go @@ -86,13 +86,14 @@ type encoder struct { // Bytes 4-1 contains offset(uint32) for Arena. // Bytes 7-6 contains attr. // Bit MSB(first bit in Byte-8) contains list field value. + // Bit SecondMSB(second bit in Byte-8) contains facetsParent field value. // Byte-5 is not getting used as of now. // |-----------------------------------------------------------------------| - // | 8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | + // | 8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | // |-----------------------------------------------------------------------| - // | MSB | | Unused | | - // | for | Attr ID | For | Offset inside Arena | - // | list | | Now | | + // | MSB - list | | Unused | | + // | SecondMSB - | Attr ID | For | Offset inside Arena | + // | facetsParent| | Now | | // |-----------------------------------------------------------------------| metaSlice []uint64 // childrenMap contains mapping of fastJsonNode to its children. @@ -149,6 +150,8 @@ func (enc *encoder) makeScalarNode(attr uint16, val []byte, list bool) (fastJson const ( // Value with most significant bit set to 1. msbBit = 0x8000000000000000 + // Value with second most significant bit set to 1. + secondMsbBit = 0x4000000000000000 // Value with all bits set to 1 for bytes 7 and 6. setBytes76 = uint64(0x00FFFF0000000000) // Compliment value of setBytes76. @@ -162,7 +165,17 @@ const ( // 1. Attr => predicate associated with this node. // 2. ScalarVal => Any value associated with node, if it is a leaf node. // 3. List => Stores boolean value, true if this node is part of list. -// 4. Children(Attrs) => List of all children. +// 4. FacetsParent => Stores boolean value, true if this node is a facetsParent. facetsParent is +// node which is parent for facets values for a scalar list predicate. Eg: node "city|country" +// will have FacetsParent value as true. +// { +// "city": ["Bengaluru", "San Francisco"], +// "city|country": { +// "0": "india", +// "1": "US" +// } +// } +// 5. Children(Attrs) => List of all children. // // All of the data for fastJsonNode tree is stored in encoder to optimise memory usage. fastJsonNode // type only stores one uint32(can be thought of id for this node). A fastJsonNode is created in @@ -217,6 +230,10 @@ func (enc *encoder) setList(fj fastJsonNode, list bool) { } } +func (enc *encoder) setFacetsParent(fj fastJsonNode) { + enc.metaSlice[fj] |= secondMsbBit +} + // appendAttrs appends attrs to existing fj's attrs. func (enc *encoder) appendAttrs(fj fastJsonNode, attrs ...fastJsonNode) { cs, ok := enc.childrenMap[fj] @@ -239,7 +256,11 @@ func (enc *encoder) getScalarVal(fj fastJsonNode) ([]byte, error) { } func (enc *encoder) getList(fj fastJsonNode) bool { - return ((enc.metaSlice[fj] & msbBit) > 0) + return (enc.metaSlice[fj] & msbBit) > 0 +} + +func (enc *encoder) getFacetsParent(fj fastJsonNode) bool { + return (enc.metaSlice[fj] & secondMsbBit) > 0 } func (enc *encoder) getAttrs(fj fastJsonNode) []fastJsonNode { @@ -522,6 +543,8 @@ func (enc *encoder) attachFacets(fj fastJsonNode, fieldName string, isList bool, if err != nil { return err } + // Mark this node as facetsParent. + enc.setFacetsParent(facetNode) enc.AddMapChild(fj, facetNode) } } @@ -636,13 +659,9 @@ func (enc *encoder) normalize(fj fastJsonNode) ([][]fastJsonNode, error) { for _, a := range fjAttrs { // Here we are counting all non-scalar attributes of fj. If there are any such // attributes, we will flatten it, otherwise we will return all attributes. - - // When we call addMapChild it tries to find whether there is already an attribute - // with attr field same as attribute argument of addMapChild. If it doesn't find any - // such attribute, it creates an attribute with isChild = false. In those cases - // sometimes cnt remains zero and normalize returns attributes without flattening. - // So we are using len(a.attrs) > 0 instead of a.isChild - if len(enc.getAttrs(a)) > 0 { + // We should only consider those nodes for flattening which have children and are not + // facetsParent. + if len(enc.getAttrs(a)) > 0 && !enc.getFacetsParent(a) { cnt++ } } @@ -658,8 +677,8 @@ func (enc *encoder) normalize(fj fastJsonNode) ([][]fastJsonNode, error) { // merged with children later. attrs := make([]fastJsonNode, 0, len(fjAttrs)-cnt) for _, a := range fjAttrs { - // Check comment at previous occurrence of len(a.attrs) > 0 - if len(enc.getAttrs(a)) == 0 { + // Here, add all nodes which have either no children or they are facetsParent. + if len(enc.getAttrs(a)) == 0 || enc.getFacetsParent(a) { attrs = append(attrs, a) } } @@ -667,8 +686,8 @@ func (enc *encoder) normalize(fj fastJsonNode) ([][]fastJsonNode, error) { for ci := 0; ci < len(fjAttrs); { childNode := fjAttrs[ci] - // Check comment at previous occurrence of len(a.attrs) > 0 - if len(enc.getAttrs(childNode)) == 0 { + // Here, exclude all nodes which have either no children or they are facetsParent. + if len(enc.getAttrs(childNode)) == 0 || enc.getFacetsParent(childNode) { ci++ continue } diff --git a/query/query_facets_test.go b/query/query_facets_test.go index e208c57a78c..2d161ea11d7 100644 --- a/query/query_facets_test.go +++ b/query/query_facets_test.go @@ -2048,6 +2048,192 @@ func TestFacetValueListPredicate(t *testing.T) { `, js) } +func TestFacetUIDPredicateWithNormalize(t *testing.T) { + populateClusterWithFacets() + query := `{ + q(func: uid(0x1)) @normalize { + name: name + from: boss @facets { + boss: name + } + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, ` + { + "data": { + "q": [ + { + "boss": "Roger", + "from|company": "company1", + "name": "Michonne" + } + ] + } + } + `, js) +} + +func TestFacetUIDListPredicateWithNormalize(t *testing.T) { + populateClusterWithFacets() + query := `{ + q(func: uid(0x1)) @normalize { + name: name + friend @facets(since) { + friend_name: name + } + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, ` + { + "data": { + "q": [ + { + "friend_name": "Rick Grimes", + "friend|since": "2006-01-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Glenn Rhee", + "friend|since": "2004-05-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Daryl Dixon", + "friend|since": "2007-05-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Andrea", + "friend|since": "2006-01-02T15:04:05Z", + "name": "Michonne" + } + ] + } + } + `, js) +} + +func TestNestedFacetUIDListPredicateWithNormalize(t *testing.T) { + populateClusterWithFacets() + query := `{ + q(func: uid(0x1)) @normalize { + name: name + friend @facets(since) @normalize { + friend_name: name @facets + friend @facets(close) { + friend_name_level2: name + } + } + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, ` + { + "data": { + "q": [ + { + "friend_name": "Rick Grimes", + "friend_name_level2": "Michonne", + "friend_name|dummy": true, + "friend_name|origin": "french", + "friend|since": "2006-01-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Glenn Rhee", + "friend_name|dummy": true, + "friend_name|origin": "french", + "friend|since": "2004-05-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Daryl Dixon", + "friend|since": "2007-05-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Andrea", + "friend_name_level2": "Michonne", + "friend|close": false, + "friend|since": "2006-01-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Andrea", + "friend_name_level2": "Glenn Rhee", + "friend|since": "2006-01-02T15:04:05Z", + "name": "Michonne" + }, + { + "friend_name": "Andrea", + "friend_name_level2": "Daryl Dixon", + "friend|close": false, + "friend|since": "2006-01-02T15:04:05Z", + "name": "Michonne" + } + ] + } + } + `, js) +} + +func TestFacetValuePredicateWithNormalize(t *testing.T) { + populateClusterWithFacets() + query := `{ + q(func: uid(1, 12000)) @normalize { + eng_name: name@en @facets + alt_name: alt_name @facets + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, ` + { + "data":{ + "q":[ + { + "eng_name|origin":"french", + "eng_name":"Michelle", + "alt_name|dummy":{ + "0":true, + "1":false + }, + "alt_name|origin":{ + "0":"french", + "1":"spanish" + }, + "alt_name|isNick":{ + "1":true + }, + "alt_name":[ + "Michelle", + "Michelin" + ] + }, + { + "eng_name|dummy":true, + "eng_name|origin":"french", + "eng_name":"Harry", + "alt_name|dummy":{ + "0":false + }, + "alt_name|isNick":{ + "0":true + }, + "alt_name|origin":{ + "0":"spanish" + }, + "alt_name":[ + "Potter" + ] + } + ] + } + } + `, js) +} + func TestFacetValueListPredicateSingleFacet(t *testing.T) { populateClusterWithFacets() query := `{