Skip to content

Commit

Permalink
(fix/dgraph): Fix facets response with normalize (#5690)
Browse files Browse the repository at this point in the history 
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> <name> "Alice" .
<0x1> <friend> "Bob" (from="college") .
<0x1> <friend> "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.
  • Loading branch information
@ashish-goswami
ashish-goswami authored Jul 9, 2020
1 parent f2773c8 commit f4c28b8
Show file tree
Hide file tree
Showing 2 changed files with 222 additions and 17 deletions.
53 changes: 36 additions & 17 deletions query/outputnode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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++
}
}
Expand All @@ -658,17 +677,17 @@ 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)
}
}
parentSlice = append(parentSlice, attrs)

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
}
Expand Down
186 changes: 186 additions & 0 deletions query/query_facets_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 := `{
Expand Down

0 comments on commit f4c28b8

Please sign in to comment.