Fix tests, fix lte binary search lookup

packages/gatsby/src/redux/nodes.ts

@@ -396,7 +396,7 @@ function addNodeToBucketWithElemMatch(
 const binarySearch = (
   values: Array<FilterValue>,
   needle: FilterValue
-): number => {
+): [number, number] => {
   let min = 0
   let max = values.length - 1
   let pivot = Math.floor(values.length / 2)
@@ -413,33 +413,30 @@ const binarySearch = (
     } else {
       // This means needle === value
       // TODO: except for NaN ... and potentially certain type casting cases
-      return pivot
+      return [pivot, pivot]
     }
 
-    if (min === max) {
+    if (max - min <= 1) {
       // End of search. Needle not found (as expected). Use pivot as index.
-      return pivot
+      // If the needle was not found, max-min==1 and max is returned.
+      return [min, max]
     }
 
     pivot = Math.floor((max - min) / 2)
   }
 
   // Shouldn't be reachable
-  return 0
+  return [0, 0]
 }
 
 /**
- * Given a ("flat") filter path leading up to "eq", a target value to filter
- * for, a set of node types, and a pre-generated lookup cache, return the set
- * of Nodes (or, if the property is `id` just the Node) which pass the filter.
- * This returns `undefined` if there is Node that passes the filter.
+ * Given the cache key for a filter and a target value return the set of nodes
+ * that resolve to this value.
+ * This returns `undefined` if there is no such node
  *
  * Basically if the filter was {a: {b: {slug: {eq: "foo/bar"}}}} then it will
  * return all the nodes that have `node.slug === "foo/bar"`. That usually (but
  * not always) at most one node for slug, but this filter can apply to anything.
- *
- * The only exception is `id`, since internally there can be at most one node
- * per `id` so there's a minor optimization for that (no need for Sets).
  */
 export const getNodesFromCacheByValue = (
   filterCacheKey: FilterCacheKey,
@@ -473,14 +470,21 @@ export const getNodesFromCacheByValue = (
 
     // Note: for lte, the valueAsc array must be set at this point
     const values = filterCache.meta.valuesAsc as Array<FilterValue>
-    const pivot = binarySearch(values, filterValue)
+    // It shouldn't find the targetValue (but it might) and return the index of
+    // the two value between which targetValue sits, or first/last element.
+    const [pivotMin, pivotMax] = binarySearch(values, filterValue)
+    // Each pivot index must have a value and a range
+    // The returned min/max index may include the lower/upper bound, so we still
+    // have to do lte checks for both values.
+    let pivotValue = values[pivotMax]
+    if (pivotValue > filterValue) {
+      pivotValue = values[pivotMin]
+    }
 
     // Note: the pivot value _shouldnt_ match the filter value because that
     // means the value was actually found, but those should have been indexed
     // so should have yielded a result in the .get() above.
 
-    // Each pivot index must have a value and a range
-    const pivotValue = values[pivot]
     const [start, stop] = filterCache.meta.valueRanges!.get(pivotValue) as [
       number,
       number

packages/gatsby/src/redux/run-sift.js

@@ -141,6 +141,9 @@ function handleMany(siftArgs, nodes) {
  * filter.
  * Only nodes of given node types will be considered
  * A fast index is created if one doesn't exist yet so cold call is slower.
+ * Returns undefined if an op was not supported for fast indexes or when no
+ * nodes were found for given (query) value. In the zero nodes case, we have to
+ * go through Sift to make sure we're not missing an edge case, for now.
  *
  * @param {Array<DbQuery>} filters Resolved. (Should be checked by caller to exist)
  * @param {Array<string>} nodeTypeNames
@@ -184,6 +187,9 @@ const runFiltersWithoutSift = (filters, nodeTypeNames, filtersCache) => {
   // Consider the case of {a: {eq: 5}, b: {eq: 10}}, do we cache the [5,10]
   // case for all value pairs? How likely is that to ever be reused?
 
+  if (result.length === 0) {
+    return undefined
+  }
   return result
 }
 
@@ -487,8 +493,7 @@ const filterWithoutSift = (filters, nodeTypeNames, filtersCache) => {
 
   if (filters.length === 0) {
     // If no filters are given, go through Sift. This does not appear to be
-    // slower than s
-    // hortcutting it here.
+    // slower than shortcutting it here.
     return undefined
   }
 

packages/gatsby/src/schema/__tests__/run-query.js

@@ -216,9 +216,11 @@ function resetDb(nodes) {
   )
 }
 
+let nodesAfterLastRunQuery
 async function runQuery(queryArgs, filtersCache) {
   const nodes = makeNodes()
   resetDb(nodes)
+  nodesAfterLastRunQuery = nodes
   const { sc, type: gqlType } = makeGqlType(nodes)
   const args = {
     gqlType,
@@ -372,8 +374,7 @@ it(`should use the cache argument`, async () => {
         let result = await runFilter({ hair: { lt: 2 } })
 
         expect(result.length).toEqual(2)
-        expect(result[0].hair).toEqual(1)
-        expect(result[1].hair).toEqual(0)
+        result.forEach(r => expect(r.hair <= 2).toBe(true))
       })
 
       it(`handles lt operator with null`, async () => {
@@ -388,30 +389,51 @@ it(`should use the cache argument`, async () => {
       it(`handles lte operator with number`, async () => {
         let result = await runFilter({ hair: { lte: 1 } })
 
-        expect(result.length).toEqual(2)
-        expect(result[0].hair).toEqual(1)
-        expect(result[1].hair).toEqual(0)
+        let actual = nodesAfterLastRunQuery.reduce(
+          (acc, node) => (node.hair <= 1 ? acc + 1 : acc),
+          0
+        )
+
+        expect(actual).not.toBe(0) // Test should keep this invariant!
+        expect(result.length).toEqual(actual)
+        result.forEach(r => expect(r.hair <= 1).toBe(true))
       })
 
       it(`should lte when value is lower than all found values`, async () => {
         if (IS_LOKI) return
 
         let result = await runFilter({ float: { lte: 1 } })
 
-        expect(result).toEqual(null) // Zero results yields undefined
+        let actual = nodesAfterLastRunQuery.reduce(
+          (acc, node) => (node.float <= 1 ? acc + 1 : acc),
+          0
+        )
+
+        expect(actual).toEqual(0) // Make sure test nodes keep this invariant!
+        expect(result).toEqual(null) // Zero results yields null
       })
 
       it(`should lte when value is in the middle of all found values`, async () => {
         let result = await runFilter({ float: { lte: 2 } })
 
-        expect(result.length > 0).toEqual(true)
+        let actual = nodesAfterLastRunQuery.reduce(
+          (acc, node) => (node.float <= 2 ? acc + 1 : acc),
+          0
+        )
+
+        expect(result.length).toEqual(actual)
         result.forEach(r => expect(r.float <= 2).toBe(true))
       })
 
       it(`should lte when value is higher than all found values`, async () => {
         let result = await runFilter({ float: { lte: 5 } })
 
-        expect(result.length).toEqual(3)
+        let actual = nodesAfterLastRunQuery.reduce(
+          (acc, node) => (node.float <= 5 ? acc + 1 : acc),
+          0
+        )
+
+        expect(result.length).toEqual(actual)
       })
 
       it.skip(`should lte when type coercion fails direct value lookup`, async () => {
@@ -420,9 +442,14 @@ it(`should use the cache argument`, async () => {
         // value wasn't mapped, that it can't be found.
         let result = await runFilter({ float: { lte: `1.5` } })
 
+        let actual = nodesAfterLastRunQuery.reduce(
+          (acc, node) => (node.float <= 1.5 ? acc + 1 : acc),
+          0
+        )
+
         expect(result).not.toBe(undefined)
         expect(result).not.toBe(null)
-        expect(result.length > 0).toEqual(true)
+        expect(result.length).toEqual(actual)
         result.forEach(r => expect(r.float <= 2).toBe(true))
       })
 
@@ -431,8 +458,14 @@ it(`should use the cache argument`, async () => {
 
         let result = await runFilter({ nil: { lte: null } })
 
+        let actual = nodesAfterLastRunQuery.reduce(
+          (acc, node) => (node.nil <= null ? acc + 1 : acc),
+          0
+        )
+
         // lte null matches null but no nodes without the property (NULL)
-        expect(result.length).toEqual(1)
+        expect(actual).not.toBe(0) // Test should keep this invariant!
+        expect(result.length).toEqual(actual)
         expect(result[0].name).toEqual(`The Mad Wax`)
         expect(result[0].nil).toEqual(null)
       })