fix: make defaultComparator provide a strict partial order

Author: Craig Macomber

b+tree.d.ts

@@ -5,11 +5,36 @@ export declare type EditRangeResult<V, R = number> = {
     break?: R;
     delete?: boolean;
 };
-/** Compares two numbers, strings, arrays of numbers/strings, Dates,
- *  or objects that have a valueOf() method returning a number or string.
- *  Optimized for numbers. Returns 1 if a>b, -1 if a<b, and 0 if a===b.
+/**
+ * numbers, strings and arrays thereof, and objects that have a valueOf() method returning a number or string like Dates.
+ */
+export declare type DefaultComparable = number | string | (number | string)[] | {
+    valueOf: () => number | string | (number | string)[];
+};
+/**
+ * Compares DefaultComparables to form a strict partial ordering.
+ *
+ * Handles +/-0 and NaN like Map: NaN is equal to NaN, and -0 is equal to +0.
+ *
+ * Arrays are compared using '<' and '>', which may cause unexpected equality: for example [1] will be considered equal to ['1'].
+ *
+ * Two objects with equal valueOf compare the same, but compare unequal to primitives that have the same value.
+ */
+export declare function defaultComparator(a: DefaultComparable, b: DefaultComparable): number;
+/**
+ * Compares finite numbers to form a strict partial ordering.
+ *
+ * Handles +/-0 like Map: -0 is equal to +0.
+ */
+export declare function compareFiniteNumbers(a: number, b: number): number;
+/**
+ * Compares strings lexically to form a strict partial ordering.
  */
-export declare function defaultComparator(a: any, b: any): number;
+export declare function compareStrings(a: string, b: string): number;
+/**
+ * If a and b are arrays, they are compared using '<' and '>', which may cause unexpected equality, for example [1] will be considered equal to ['1'].
+ */
+export declare function compareFiniteNumbersOrStringOrArray(a: number | string | (number | string)[], b: number | string | (number | string)[]): number;
 /**
  * A reasonably fast collection of key-value pairs with a powerful API.
  * Largely compatible with the standard Map. BTree is a B+ tree data structure,
@@ -78,11 +103,15 @@ export default class BTree<K = any, V = any> implements ISortedMapF<K, V>, ISort
     private _root;
     _size: number;
     _maxNodeSize: number;
+    /**
+     * provides a total order over keys (and a strict partial order over the type K)
+     * @returns a negative value if a < b, 0 if a === b and a positive value if a > b
+     */
     _compare: (a: K, b: K) => number;
     /**
      * Initializes an empty B+ tree.
      * @param compare Custom function to compare pairs of elements in the tree.
-     *   This is not required for numbers, strings and arrays of numbers/strings.
+     *   If not specified, defaultComparator will be used which is valid as long as K extends DefaultComparable.
      * @param entries A set of key-value pairs to initialize the tree
      * @param maxNodeSize Branching factor (maximum items or children per node)
      *   Must be in range 4..256. If undefined or <4 then default is used; if >256 then 256.

b+tree.js

@@ -13,44 +13,90 @@ var __extends = (this && this.__extends) || (function () {
     };
 })();
 Object.defineProperty(exports, "__esModule", { value: true });
-exports.EmptyBTree = exports.defaultComparator = void 0;
-// Informative microbenchmarks & stuff:
-// http://www.jayconrod.com/posts/52/a-tour-of-v8-object-representation (very educational)
-// https://blog.mozilla.org/luke/2012/10/02/optimizing-javascript-variable-access/ (local vars are faster than properties)
-// http://benediktmeurer.de/2017/12/13/an-introduction-to-speculative-optimization-in-v8/ (other stuff)
-// https://jsperf.com/js-in-operator-vs-alternatives (avoid 'in' operator; `.p!==undefined` faster than `hasOwnProperty('p')` in all browsers)
-// https://jsperf.com/instanceof-vs-typeof-vs-constructor-vs-member (speed of type tests varies wildly across browsers)
-// https://jsperf.com/detecting-arrays-new (a.constructor===Array is best across browsers, assuming a is an object)
-// https://jsperf.com/shallow-cloning-methods (a constructor is faster than Object.create; hand-written clone faster than Object.assign)
-// https://jsperf.com/ways-to-fill-an-array (slice-and-replace is fastest)
-// https://jsperf.com/math-min-max-vs-ternary-vs-if (Math.min/max is slow on Edge)
-// https://jsperf.com/array-vs-property-access-speed (v.x/v.y is faster than a[0]/a[1] in major browsers IF hidden class is constant)
-// https://jsperf.com/detect-not-null-or-undefined (`x==null` slightly slower than `x===null||x===undefined` on all browsers)
-// Overall, microbenchmarks suggest Firefox is the fastest browser for JavaScript and Edge is the slowest.
-// Lessons from https://v8project.blogspot.com/2017/09/elements-kinds-in-v8.html:
-//   - Avoid holes in arrays. Avoid `new Array(N)`, it will be "holey" permanently.
-//   - Don't read outside bounds of an array (it scans prototype chain).
-//   - Small integer arrays are stored differently from doubles
-//   - Adding non-numbers to an array deoptimizes it permanently into a general array
-//   - Objects can be used like arrays (e.g. have length property) but are slower
-//   - V8 source (NewElementsCapacity in src/objects.h): arrays grow by 50% + 16 elements
-/** Compares two numbers, strings, arrays of numbers/strings, Dates,
- *  or objects that have a valueOf() method returning a number or string.
- *  Optimized for numbers. Returns 1 if a>b, -1 if a<b, and 0 if a===b.
+exports.EmptyBTree = exports.compareFiniteNumbersOrStringOrArray = exports.compareStrings = exports.compareFiniteNumbers = exports.defaultComparator = void 0;
+/**
+ * Compares DefaultComparables to form a strict partial ordering.
+ *
+ * Handles +/-0 and NaN like Map: NaN is equal to NaN, and -0 is equal to +0.
+ *
+ * Arrays are compared using '<' and '>', which may cause unexpected equality: for example [1] will be considered equal to ['1'].
+ *
+ * Two objects with equal valueOf compare the same, but compare unequal to primitives that have the same value.
  */
 function defaultComparator(a, b) {
-    var c = a - b;
-    if (c === c)
-        return c; // a & b are number
-    // General case (c is NaN): string / arrays / Date / incomparable things
-    if (a)
+    // Compare types first.
+    // Note that the trick of using 'a - b' the checking for NaN to detect non numbers values does not work if the strings are numeric (ex: "5"),
+    // leading most comparison functions using that approach to fail to have transitivity.
+    var ta = typeof a;
+    var tb = typeof b;
+    if (ta !== tb) {
+        return ta < tb ? -1 : 1;
+    }
+    if (ta === 'object') {
         a = a.valueOf();
-    if (b)
         b = b.valueOf();
-    return a < b ? -1 : a > b ? 1 : a == b ? 0 : c;
+        ta = typeof a;
+        tb = typeof b;
+        // Deal with one producing a string, and the other a number
+        if (ta !== tb) {
+            return ta < tb ? -1 : 1;
+        }
+    }
+    // a and b are now the same type, and either a number, string or array.
+    // use Object.is to make NaN compare equal to NaN.
+    // This treats also -0 as not equal to 0, which is handled separately below.
+    if (Object.is(a, b))
+        return 0;
+    // All comparisons with NaN return false, so NaNs will pass here.
+    if (a < b)
+        return -1;
+    // Since a and b might be arrays, we cannot rely on === or ==, only < and > do something useful for ordering arrays.
+    // To find if two arrays are equal using comparison operators, both < and > must be checked (even == returns false if not the same object).
+    if (a > b)
+        return 1;
+    // Order NaN less than other numbers
+    if (Number.isNaN(a))
+        return -1;
+    if (Number.isNaN(b))
+        return 1;
+    // Handles 0 and -0 case, as well as equal (but not same object) arrays case.
+    return 0;
 }
 exports.defaultComparator = defaultComparator;
 ;
+/**
+ * Compares finite numbers to form a strict partial ordering.
+ *
+ * Handles +/-0 like Map: -0 is equal to +0.
+ */
+function compareFiniteNumbers(a, b) {
+    return a - b;
+}
+exports.compareFiniteNumbers = compareFiniteNumbers;
+;
+/**
+ * Compares strings lexically to form a strict partial ordering.
+ */
+function compareStrings(a, b) {
+    return a > b ? 1 : a === b ? 0 : -1;
+}
+exports.compareStrings = compareStrings;
+;
+/**
+ * If a and b are arrays, they are compared using '<' and '>', which may cause unexpected equality, for example [1] will be considered equal to ['1'].
+ */
+function compareFiniteNumbersOrStringOrArray(a, b) {
+    // Strings can not be ordered relative to numbers using '<' and '>' since no matter the order, the comparison will return false.
+    var ta = typeof a;
+    var tb = typeof b;
+    if (ta !== tb) {
+        return ta < tb ? -1 : 1;
+    }
+    // Use < and > instead of < and === so arrays work correctly.
+    return a > b ? 1 : a < b ? -1 : 0;
+}
+exports.compareFiniteNumbersOrStringOrArray = compareFiniteNumbersOrStringOrArray;
+;
 /**
  * A reasonably fast collection of key-value pairs with a powerful API.
  * Largely compatible with the standard Map. BTree is a B+ tree data structure,
@@ -119,7 +165,7 @@ var BTree = /** @class */ (function () {
     /**
      * Initializes an empty B+ tree.
      * @param compare Custom function to compare pairs of elements in the tree.
-     *   This is not required for numbers, strings and arrays of numbers/strings.
+     *   If not specified, defaultComparator will be used which is valid as long as K extends DefaultComparable.
      * @param entries A set of key-value pairs to initialize the tree
      * @param maxNodeSize Branching factor (maximum items or children per node)
      *   Must be in range 4..256. If undefined or <4 then default is used; if >256 then 256.

b+tree.test.ts

@@ -1,4 +1,4 @@
-import BTree, {IMap, EmptyBTree} from './b+tree';
+import BTree, {IMap, EmptyBTree, defaultComparator, compareFiniteNumbers, compareFiniteNumbersOrStringOrArray, compareStrings} from './b+tree';
 import SortedArray from './sorted-array';
 import MersenneTwister from 'mersenne-twister';
 
@@ -14,6 +14,153 @@ function addToBoth<K,V>(a: IMap<K,V>, b: IMap<K,V>, k: K, v: V) {
   expect(a.set(k,v)).toEqual(b.set(k,v));
 }
 
+describe('defaultComparator', () =>
+{
+  const dateA = new Date(Date.UTC(96, 1, 2, 3, 4, 5));
+  const dateA2 = new Date(Date.UTC(96, 1, 2, 3, 4, 5));
+  const dateB = new Date(Date.UTC(96, 1, 2, 3, 4, 6));
+  const values = [
+    dateA,
+    dateA2,
+    dateB,
+    dateA.valueOf(),
+    '24x',
+    '0',
+    '1',
+    '3',
+    'String',
+    '10',
+    0,
+    "NaN",
+    NaN,
+    Infinity,
+    -0,
+    -Infinity,
+    1,
+    10,
+    2,
+    [],
+    '[]',
+    [1],
+    ['1']
+  ];
+  const sorted = [-Infinity, -10, -1, -0, 0, 1, 2, 10, Infinity];
+  testComparison(defaultComparator, sorted, values, [[dateA, dateA2], [0, -0], [[1], ['1']]]);
+});
+
+describe('compareFiniteNumbers', () =>
+{
+  const sorted = [-10, -1, -0, 0, 1, 2, 10];
+  testComparison(compareFiniteNumbers, sorted, sorted, [[-0, 0]]);
+});
+
+describe('compareStrings', () =>
+{
+  const values = [
+    '24x',
+    '+0',
+    '0.0',
+    '0',
+    '-0',
+    '1',
+    '3',
+    'String',
+    '10',
+    "NaN",
+  ];;
+  testComparison(compareStrings, [], values, []);
+});
+
+describe('compareFiniteNumbersOrStringOrArray', () =>
+{
+  const values = [
+    '24x',
+    '0',
+    '1',
+    '3',
+    'String',
+    '10',
+    0,
+    "NaN",
+    -0,
+    1,
+    10,
+    2,
+    [],
+    '[]',
+    [1],
+    ['1']
+  ];
+  const sorted = [-10, -1, -0, 0, 1, 2, 10];
+  testComparison(compareFiniteNumbersOrStringOrArray, sorted, values, [[0, -0], [[1], ['1']]]);
+});
+
+
+/**
+ * Tests a comparison function, ensuring it produces a strict partial order over the provided values.
+ * Additionally confirms that the comparison function has the correct definition of equality via expectedDuplicates.
+ */
+function testComparison(comparison: (a: any, b: any) => number, inOrder: any[], values: any[], expectedDuplicates: [any, any][] = []) {
+  function check(a: any, b: any): number {
+    const v = comparison(a, b);
+    expect(typeof v).toEqual('number');
+    expect(v === v).toEqual(true); // Not NaN
+    return Math.sign(v);
+  }
+
+  test('comparison has correct order', () => {
+    expect([...inOrder].sort(comparison)).toMatchObject(inOrder);
+  });
+
+  test('comparison deffierantes values', () => {
+    let duplicates = [];
+    for (let i = 0; i < values.length; i++) {
+      for (let j = i + 1; j < values.length; j++) {
+        if (check(values[i], values[j]) === 0) {
+          duplicates.push([values[i], values[j]]);
+        }
+      }
+    }
+    expect(duplicates).toMatchObject(expectedDuplicates);
+  });
+
+  test('comparison forms a strict partial ordering', () => {
+    // To be a strict partial order, the function must be:
+    // irreflexive: not a < a
+    // transitive: if a < b and b < c then a < c
+    // asymmetric: if a < b then not b < a
+
+    // Since our comparison has three outputs, we adjust that to, we need to tighten the rules that involve 'not a < b' (where we have two possible outputs) as follows:
+    // irreflexive: compare(a, a) === 0
+    // transitive: if compare(a, b) < 0 and compare(b, c) < 0 then compare(a, c) < 0
+    // asymmetric: sign(compare(a, b)) === -sign(compare(b, a))
+
+    // This can is brute forced in O(n^3) time below:
+    // Violations
+    const irreflexive = []
+    const transitive = []
+    const asymmetric = []
+    for (const a of values) {
+      // irreflexive: compare(a, a) === 0
+      if(check(a, a) !== 0) irreflexive.push(a);
+      for (const b of values) {
+        for (const c of values) {
+          // transitive: if compare(a, b) < 0 and compare(b, c) < 0 then compare(a, c) < 0
+          if (check(a, b) < 0 && check(b, c) < 0) {
+            if(check(a, c) !== -1) transitive.push([a, b, c]);
+          }
+        }
+        // sign(compare(a, b)) === -sign(compare(b, a))
+        if(check(a, b) !== -check(b, a)) asymmetric.push([a, b]);
+      }
+    }
+    expect(irreflexive).toEqual([]);
+    expect(transitive).toEqual([]);
+    expect(asymmetric).toEqual([]);
+  });
+}
+
+
 describe('Simple tests on leaf nodes', () =>
 {
   test('A few insertions (fanout 8)', insert8.bind(null, 8));
@@ -427,4 +574,33 @@ function testBTree(maxNodeSize: number)
     expect(tree.nextLowerPair(undefined)).toEqual([300, 600]);
     expect(tree.nextHigherPair(undefined)).toEqual([-10, -20]);
   });
+
+  test('Regression test for invalid default comparator causing malformed trees', () => {
+    const key = '24e26f0b-3c1a-47f8-a7a1-e8461ddb69ce6';
+    const tree = new BTree<string,{}>(undefined, undefined, maxNodeSize);
+    // The defaultComparator was not transitive for these inputs due to comparing numeric strings to each other numerically,
+    // but lexically when compared to non-numeric strings. This resulted in keys not being orderable, and the tree behaving incorrectly.
+    const inputs: [string,{}][] = [
+      [key, {}],
+      ['0', {}],
+      ['1', {}],
+      ['2', {}],
+      ['3', {}],
+      ['4', {}],
+      ['Cheese', {}],
+      ['10', {}],
+      ['11', {}],
+      ['12', {}],
+      ['13', {}],
+      ['15', {}],
+      ['16', {}],
+    ];
+
+    for (const [id, node] of inputs) {
+      expect( tree.set(id, node)).toBeTruthy();
+      tree.checkValid();
+      expect(tree.get(key)).not.toBeUndefined();
+    }
+    expect(tree.get(key)).not.toBeUndefined();
+  });
 }