Merge Iterator alternate implementation

[jarifibrahim]

This PR introduces an alternate implementation of #1045

Benchmarks

master-x.txt and new-x.txt are the result of ReadMerged benchmarks with x tables.
Each table contains 10e6 entries.

benchstat master-5.txt new-5.txt
name           old time/op  new time/op  delta
ReadMerged-16   826ms ± 1%   470ms ± 2%  -43.14%  (p=0.008 n=5+5)

benchstat master-16.txt new-16.txt
name           old time/op  new time/op  delta
ReadMerged-16   4.22s ± 1%   2.08s ± 2%  -50.72%  (p=0.008 n=5+5)

benchstat master-32.txt new-32.txt
name           old time/op  new time/op  delta
ReadMerged-16   10.6s ± 3%    4.8s ± 2%  -54.58%  (p=0.008 n=5+5)

benchstat master-64.txt new-64.txt
name           old time/op  new time/op  delta
ReadMerged-16   25.1s ± 1%   11.0s ± 2%  -55.94%  (p=0.008 n=5+5)

benchstat master-128.txt new-128.txt
name           old time/op  new time/op  delta
ReadMerged-16   58.5s ± 1%   25.8s ± 2%  -55.95%  (p=0.016 n=4+5)

---

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

@jarifibrahim you can click here to see the review status or cancel the code review job.

[coveralls]

Coverage decreased (-0.1%) to 77.503% when pulling 59542ea on ibrahim/mergeiterator-change into dbe1cb9 on master.

[coveralls]

Coverage decreased (-0.09%) to 77.54% when pulling ad4c19c on ibrahim/mergeiterator-change into dbe1cb9 on master.

[pullrequest]

The change looks good to me. I added a few notes about best practices and code readability.

---

Reviewed with ❤️ by PullRequest

[ashish-goswami]

Reviewed 4 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jarifibrahim and @manishrjain)

[manishrjain]

Got some comments.

Reviewed 4 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jarifibrahim and @pullrequest[bot])

---

table/merge_iterator.go, line 47 at r2 (raw file):

func (n *node) setIterator(iter y.Iterator) {
	n.iter = iter
	n.merge, _ = iter.(*MergeIterator)

Please add a comment saying that even if the conversion fails, n.merge would be nil, which is OK and you handle that.

---

table/merge_iterator.go, line 106 at r2 (raw file):

		if &mi.right == mi.small {
			mi.swapSmall()
		}

return

---

table/merge_iterator.go, line 108 at r2 (raw file):

		}
	}
	if mi.reverse {

if cmp == 0 { .. ; return}
if cmp < 0 { .. if reverse then do, else do. }
else { .. if reverse then do, else do }

---

table/merge_iterator.go, line 194 at r2 (raw file):

		mi.left.setIterator(iters[0])
		mi.right.setIterator(iters[1])
		mi.small = &mi.left

Add a comment that assign one randomly, because fix would fix this.

[jarifibrahim]

Dismissed @pullrequest[bot] from 2 discussions.
Reviewable status: 5 of 6 files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

---

table/merge_iterator.go, line 55 at r1 (raw file):

Previously, pullrequest[bot] wrote…

By actively ignoring the second argument here, you're effectively silencing the error in type check. This might be ok given that it's an unexported function, but it might be better for the long term if the second argument was either assigned and checked or left out for a panic to happen.

Done.

---

table/merge_iterator.go, line 136 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

I feel the existing code is more readable.

Done.

---

table/merge_iterator.go, line 47 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Please add a comment saying that even if the conversion fails, n.merge would be nil, which is OK and you handle that.

Done.

---

table/merge_iterator.go, line 106 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

return

Done.

---

table/merge_iterator.go, line 108 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if cmp == 0 { .. ; return}
if cmp < 0 { .. if reverse then do, else do. }
else { .. if reverse then do, else do }

Done.

---

table/merge_iterator.go, line 194 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add a comment that assign one randomly, because fix would fix this.

Done.