Skip to content

Commit

Permalink
(release/v20.03) Fix facets response with normalize (#5691)
Browse files Browse the repository at this point in the history 
Fixes: #5241
Fixes: DGRAPH-1670

This PR is related to #5690. It issues with facets response when facets are retrieved with
@normalize directive. Below two cases are covered:

Fixing facets response with uid/uid list predicates.
Fixing facets response with scalard list predicates.
  • Loading branch information
@ashish-goswami
ashish-goswami authored Jul 9, 2020
1 parent ceac69f commit 0c25a21
Show file tree
Hide file tree
Showing 2 changed files with 231 additions and 18 deletions.
63 changes: 45 additions & 18 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 @@ -662,13 +685,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 @@ -684,17 +703,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 Expand Up @@ -1125,9 +1144,17 @@ func (sg *SubGraph) preTraverse(enc *encoder, uid uint64, dst fastJsonNode) erro
return err
}
}
nonEmptyUID = append(nonEmptyUID, childIdx) // append index to nonEmptyUID.
if !pc.Params.Normalize {
nonEmptyUID = append(nonEmptyUID, childIdx) // append index to nonEmptyUID.
}

if pc.Params.Normalize {
if pc.Params.Facet != nil && len(fcsList) > childIdx {
err := enc.attachFacets(uc, fieldName, false, fcsList[childIdx].Facets, 0)
if err != nil {
return err
}
}
// We will normalize at each level instead of
// calling normalize after pretraverse.
// Now normalize() only flattens one level,
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 @@ -2134,6 +2134,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 0c25a21

Please sign in to comment.