Fix SortedMap's reverse iterator (without start key). #6020
Conversation
|
Changeset File Check
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (652,019 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
| ): void { | ||
| const countUp = from <= to; | ||
| let expected = from; | ||
| while (iterator.hasNext()) { |
There was a problem hiding this comment.
FWIW From the method contract, it seems like this method should iterate until it reaches to and validate that hasNext remains true.
There was a problem hiding this comment.
will rename to validateIteratorCoversRange
| @@ -197,7 +197,7 @@ export class SortedMapIterator<K, V> { | |||
| while (!node.isEmpty()) { | |||
| cmp = startKey ? comparator(node.key, startKey) : 1; | |||
| // flip the comparison if we're going in reverse | |||
| if (isReverse) { | |||
| if (startKey && isReverse) { | |||
There was a problem hiding this comment.
I can't get my head around this - why do we not need to do this when we iterate without start key? I can debug this to understand more, but I wonder if you have a succinct explanation why this is the correct fix.
There was a problem hiding this comment.
Sure. It boils down to the iteration logic below (line 204-227).
First, description of the bug:
If this constructor is called with isReverse = true and startKey = null, then cmp will always be -1 in all iterations of the while loop. Notice that this.nodeStack.push is never called if cmp < 0. So, reverse traversal with no startKey will always result in iterating zero nodes.
More explanation
The purpose of this while loop is to initialize a stack that will be used for traversal. The while loop always starts at the root node of a red-black tree.
In the absence of a startKey,
- For forward iteration (iterating from smallest node to largest node): the while loop should start from the root and keep going to the left child repeatedly until we reach the left-most leaf node (pushing these nodes onto the stack along the way).
- For reverse iteration (iterating from largest node to smallest node): the while loop should start from the root and keep going to the right child repeatedly until we reach the right-most leaf node (pushing these nodes onto the stack along the way).
In the presence of a startKey,
We should start at the root of the tree, and first search for the startKey. We should not push anything onto the stack until we find the startKey (or find a node that's larger than startKey for forward iteration, or find a node that's smaller than startKey for reverse iteration). At that point, we should start pushing nodes onto the stack.
There was a problem hiding this comment.
In other words, the code can also be written as follows:
if(startKey) {
while (!node.isEmpty()) {
cmp = comparator(node.key, startKey);
// flip the comparison if we're going in reverse
if (isReverse) {
cmp *= -1;
}
if (cmp < 0) {
// This node is less than our start key. ignore it
if (this.isReverse) {
node = node.left;
} else {
node = node.right;
}
} else if (cmp === 0) {
// This node is exactly equal to our start key. Push it on the stack,
// but stop iterating;
this.nodeStack.push(node);
break;
} else {
// This node is greater than our start key, add it to the stack and move
// to the next one
this.nodeStack.push(node);
if (this.isReverse) {
node = node.right;
} else {
node = node.left;
}
}
}
} else {
// There is no startKey, go all the way.
while (!node.isEmpty()) {
this.nodeStack.push(node);
if (this.isReverse) {
node = node.right;
} else {
node = node.left;
}
}
}There was a problem hiding this comment.
Okay, thanks for the explanation. I didn't quite catch that this same code was doing two completely different things (finding the start key and then building the stack). I think we could probably fix this to run two iterations, but that's a P3 and this PR is good to ship.
I happen to be the first person to use
getReverseIterator()which has a bug.