Minor changes in fastJsonNode

[ashish-goswami]

This PR further tries to simplify fastJsonNode processing. It has following changes:

• Return error response if encoded response is > 4GB in size.
• Replace idMap with idSlice in encoder.
• Refactor AddListChild and AddMapChild to remove attr from their arguments. Both functions
  accept parent, child and attr as arguments. This attr is equal to attr field of child, hence sending
  it explicitly can be avoided.
• Minimise calls to idForAttr().

---

This change is 

[pawanrawal]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashish-goswami)

---

query/outputnode.go, line 134 at r1 (raw file):

// makeScalarNode returns a fastJsonNode with all of its meta data, scalarVal populated.
func (enc *encoder) makeScalarNode(attr uint16, val []byte, list bool) (fastJsonNode, error) {
	fj := enc.newNode()

Can we remove newNode now?

[vvbalaji-dgraph]

LGTM

[vvbalaji-dgraph]

1. Would it make sense to have a query property file with such limits?
2. Why is the limit 4 GB (say why not 2 GB or 8 GB)?

[manishrjain]

Address the comments.

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ashish-goswami)

---

query/outputnode.go, line 85 at r1 (raw file):

	metaSlice []uint64
	// childrenMap contains mapping of fastJsonNode to its children.
	childrenMap map[fastJsonNode][]fastJsonNode

Somewhere here keep a running size estimate. Every thing that you add, add some estimate of what it costs.

Arena should cost.
idSlice should cost.
metaSlice should cost.

...

Based on that you can decide when to quit early.

---

query/outputnode.go, line 90 at r1 (raw file):

func newEncoder() *encoder {
	var metaSlice []uint64
	var idSlice []string

metaSlice := make([]uint64, 1)
idSlice := make([]string, 1)

Then you don't need to add empty values.

---

query/outputnode.go, line 113 at r1 (raw file):

	enc.idSlice = append(enc.idSlice, attr)
	enc.attrMap[attr] = uint16(len(enc.idSlice) - 1) // TODO(Ashish): check for overflow.

Add the check. Also, assign a var to the value, and use in both these statements.

---

query/outputnode.go, line 119 at r1 (raw file):

func (enc *encoder) attrForID(id uint16) string {
	// For now we are not returning error from here.
	if id == 0 || id > uint16(len(enc.idSlice)) {

id >= len

---

query/outputnode.go, line 787 at r1 (raw file):

	attrID := enc.idForAttr(sg.Params.Alias)
	if sg.uidMatrix == nil {
		enc.AddListChild(fj, enc.newNodeWithAttr(attrID))

Rename it to newNode.

---

query/outputnode.go, line 892 at r1 (raw file):

	// Return error if encoded buffer size exceeds than a threshold size.
	if int64(bufw.Len()) > maxEncodedSize {

This is OK. But, this comes way later. So, better to quit earlier. See suggestion above.

[ashish-goswami]

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @ashish-goswami, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

query/outputnode.go, line 90 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

metaSlice := make([]uint64, 1)
idSlice := make([]string, 1)

Then you don't need to add empty values.

Done.

---

query/outputnode.go, line 119 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

id >= len

Done.

---

query/outputnode.go, line 134 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we remove newNode now?

Done. also renamed newNodeWithAttr to newNode.

---

query/outputnode.go, line 787 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Rename it to newNode.

Done.

---

query/outputnode.go, line 858 at r1 (raw file):

Previously, vvbalaji-dgraph (V V Balaji) wrote…

1. Would it make sense to have a query property file with such limits?
2. Why is the limit 4 GB (say why not 2 GB or 8 GB)?

I have explained in the comment why it is 4GB. Also right now we have only one property and hence not adding it to a property file for now.

[manishrjain]

Reviewable status: 0 of 3 files reviewed, 9 unresolved discussions (waiting on @ashish-goswami, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

query/outputnode.go, line 79 at r2 (raw file):

	// 1. By adding predicate len, while expanding it for an uid in preTraverse().
	// 2. By adding scalarVal len in setScalarVal function for a leaf(scalar) node.
	estSize uint64

curSize

[ashish-goswami]

Reviewable status: 0 of 3 files reviewed, 9 unresolved discussions (waiting on @ashish-goswami, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

query/outputnode.go, line 79 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

curSize

Done.