From 086a7541fc7e4703ea05822431ce5300732a40ae Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 18 Feb 2022 22:42:19 -0600 Subject: [PATCH 1/3] Fix SortedMap's reverse iterator (without start key). --- .../test/unit/util/sorted_map.test.ts | 98 +++++++++++++++---- 1 file changed, 78 insertions(+), 20 deletions(-) diff --git a/packages/firestore/test/unit/util/sorted_map.test.ts b/packages/firestore/test/unit/util/sorted_map.test.ts index c52ded7403d..49c7cfd809b 100644 --- a/packages/firestore/test/unit/util/sorted_map.test.ts +++ b/packages/firestore/test/unit/util/sorted_map.test.ts @@ -19,7 +19,11 @@ import { expect } from 'chai'; import { primitiveComparator } from '../../../src/util/misc'; import { forEach } from '../../../src/util/obj'; -import { LLRBNode, SortedMap } from '../../../src/util/sorted_map'; +import { + LLRBNode, + SortedMap, + SortedMapIterator +} from '../../../src/util/sorted_map'; function shuffle(arr: number[]): void { for (let i = arr.length - 1; i > 0; i--) { @@ -32,6 +36,41 @@ function shuffle(arr: number[]): void { // Many of these were adapted from the mugs source code. // http://mads379.github.com/mugs/ describe('SortedMap Tests', () => { + /** Returns a map with (i, i) entries for i in [0, 9] inclusive */ + function getMapOfNumbersZeroToNine(): SortedMap { + const items: number[] = []; + for (let i = 0; i < 10; i++) { + items.push(i); + } + shuffle(items); + + let map = new SortedMap(primitiveComparator); + for (let i = 0; i < 10; i++) { + map = map.insert(items[i], items[i]); + } + return map; + } + + /** + * Validates that the given iterator covers the given range by counting from + * the given `from` argument to the given `to` argument (inclusive). + */ + function validateIteratesRangeInclusive( + iterator: SortedMapIterator, + from: number, + to: number + ): void { + const countUp = from <= to; + let expected = from; + while (iterator.hasNext()) { + const n = iterator.getNext(); + expect(n.key).to.equal(expected); + expect(n.value).to.equal(expected); + expected = countUp ? expected + 1 : expected - 1; + } + expect(expected).to.equal(countUp ? to + 1 : to - 1); + } + it('Create node', () => { const map = new SortedMap(primitiveComparator).insert('key', 'value'); expect(map.root.left.isEmpty()).to.equal(true); @@ -405,27 +444,46 @@ describe('SortedMap Tests', () => { expect(() => iterator.getNext()).to.throw(); }); - it('SortedMapIterator test with 10 items.', () => { - const items: number[] = []; - for (let i = 0; i < 10; i++) { - items.push(i); - } - shuffle(items); + it('forward iterator without start key', () => { + const iterator = getMapOfNumbersZeroToNine().getIterator(); + validateIteratesRangeInclusive(iterator, 0, 9); + }); - let map = new SortedMap(primitiveComparator); - for (let i = 0; i < 10; i++) { - map = map.insert(items[i], items[i]); - } + it('forward iterator with start key.', () => { + const iterator = getMapOfNumbersZeroToNine().getIteratorFrom(5); + validateIteratesRangeInclusive(iterator, 5, 9); + }); - const iterator = map.getIterator(); - let expected = 0; - while (iterator.hasNext()) { - const n = iterator.getNext(); - expect(n.key).to.equal(expected); - expect(n.value).to.equal(expected); - expected++; - } - expect(expected).to.equal(10); + it('forward iterator with start key larger than max key', () => { + const iterator = getMapOfNumbersZeroToNine().getIteratorFrom(50); + expect(iterator.hasNext()).to.equal(false); + expect(() => iterator.getNext()).to.throw(); + }); + + it('forward iterator with start key smaller than min key', () => { + const iterator = getMapOfNumbersZeroToNine().getIteratorFrom(-50); + validateIteratesRangeInclusive(iterator, 0, 9); + }); + + it('reverse iterator without start key', () => { + const iterator = getMapOfNumbersZeroToNine().getReverseIterator(); + validateIteratesRangeInclusive(iterator, 9, 0); + }); + + it('reverse iterator with start key.', () => { + const iterator = getMapOfNumbersZeroToNine().getReverseIteratorFrom(7); + validateIteratesRangeInclusive(iterator, 7, 0); + }); + + it('reverse iterator with start key smaller than min key', () => { + const iterator = getMapOfNumbersZeroToNine().getReverseIteratorFrom(-50); + expect(iterator.hasNext()).to.equal(false); + expect(() => iterator.getNext()).to.throw(); + }); + + it('reverse iterator with start key larger than max key', () => { + const iterator = getMapOfNumbersZeroToNine().getReverseIteratorFrom(50); + validateIteratesRangeInclusive(iterator, 9, 0); }); it('SortedMap.indexOf returns index.', () => { From 5ca49914346f99373c7c97717ad9a7bdf99850b6 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 18 Feb 2022 23:02:41 -0600 Subject: [PATCH 2/3] Fix sorted map. --- packages/firestore/src/util/sorted_map.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/util/sorted_map.ts b/packages/firestore/src/util/sorted_map.ts index 0c144066ce0..a24cf8802ca 100644 --- a/packages/firestore/src/util/sorted_map.ts +++ b/packages/firestore/src/util/sorted_map.ts @@ -197,7 +197,7 @@ export class SortedMapIterator { while (!node.isEmpty()) { cmp = startKey ? comparator(node.key, startKey) : 1; // flip the comparison if we're going in reverse - if (isReverse) { + if (startKey && isReverse) { cmp *= -1; } From 552f3e531ce51278c1d37ef9d4bed6cd8bfe5ca4 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 23 Feb 2022 16:39:00 -0600 Subject: [PATCH 3/3] address comments. --- .../firestore/test/unit/util/sorted_map.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/firestore/test/unit/util/sorted_map.test.ts b/packages/firestore/test/unit/util/sorted_map.test.ts index 49c7cfd809b..7308268e2e9 100644 --- a/packages/firestore/test/unit/util/sorted_map.test.ts +++ b/packages/firestore/test/unit/util/sorted_map.test.ts @@ -55,7 +55,7 @@ describe('SortedMap Tests', () => { * Validates that the given iterator covers the given range by counting from * the given `from` argument to the given `to` argument (inclusive). */ - function validateIteratesRangeInclusive( + function validateIteratorCoversRange( iterator: SortedMapIterator, from: number, to: number @@ -446,12 +446,12 @@ describe('SortedMap Tests', () => { it('forward iterator without start key', () => { const iterator = getMapOfNumbersZeroToNine().getIterator(); - validateIteratesRangeInclusive(iterator, 0, 9); + validateIteratorCoversRange(iterator, 0, 9); }); it('forward iterator with start key.', () => { const iterator = getMapOfNumbersZeroToNine().getIteratorFrom(5); - validateIteratesRangeInclusive(iterator, 5, 9); + validateIteratorCoversRange(iterator, 5, 9); }); it('forward iterator with start key larger than max key', () => { @@ -462,17 +462,17 @@ describe('SortedMap Tests', () => { it('forward iterator with start key smaller than min key', () => { const iterator = getMapOfNumbersZeroToNine().getIteratorFrom(-50); - validateIteratesRangeInclusive(iterator, 0, 9); + validateIteratorCoversRange(iterator, 0, 9); }); it('reverse iterator without start key', () => { const iterator = getMapOfNumbersZeroToNine().getReverseIterator(); - validateIteratesRangeInclusive(iterator, 9, 0); + validateIteratorCoversRange(iterator, 9, 0); }); it('reverse iterator with start key.', () => { const iterator = getMapOfNumbersZeroToNine().getReverseIteratorFrom(7); - validateIteratesRangeInclusive(iterator, 7, 0); + validateIteratorCoversRange(iterator, 7, 0); }); it('reverse iterator with start key smaller than min key', () => { @@ -483,7 +483,7 @@ describe('SortedMap Tests', () => { it('reverse iterator with start key larger than max key', () => { const iterator = getMapOfNumbersZeroToNine().getReverseIteratorFrom(50); - validateIteratesRangeInclusive(iterator, 9, 0); + validateIteratorCoversRange(iterator, 9, 0); }); it('SortedMap.indexOf returns index.', () => {