Pu/lookupfix (#791)

* removing option, fixing tests * changelog * restore option * changelog * linter
handsontable · Aug 3, 2021 · f411eba · f411eba
1 parent b303691
commit f411eba
Show file tree

Hide file tree

Showing 10 changed files with 104 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,7 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+- `binarySearchThreshold` option is now deprecated, as every search of sorted data always uses binary search.
+
 ### Fixed
+- Fixed issue with searching sorted data (#787).
 - Added new CellType returned by `getCellType`, CellType.ARRAYFORMULA, assigned to top-left corner of arrays. Recognized by `isCellPartOfArray` and `doesCellHaveFormula`. (#781)
 
 ### Added

diff --git a/src/Config.ts b/src/Config.ts
@@ -41,10 +41,8 @@ export interface ConfigParams {
   accentSensitive: boolean,
   /**
    * Sets a minimum number of elements that a range must have to use binary search.
-   * 
-   * Shorter ranges are searched naively.
-   * 
-   * Used by the VLOOKUP, HLOOKUP and MATCH functions.
+   *
+   * @deprecated Every search of sorted data always uses binary search.
    *
    * @default 20
    *
@@ -687,6 +685,7 @@ export class Config implements ConfigParams, ParserConfig {
       }
     })
     validateNumberToBeAtLeast(this.maxColumns, 'maxColumns', 1)
+    this.warnDeprecatedIfUsed(binarySearchThreshold, 'binarySearchThreshold', '1.1')
 
     privatePool.set(this, {
       licenseKeyValidityState: checkLicenseKeyValidity(this.licenseKey)
@@ -724,9 +723,13 @@ export class Config implements ConfigParams, ParserConfig {
     return new Config(mergedConfig)
   }
 
-  private warnDeprecatedIfUsed(inputValue: any, paramName: string, fromVersion: string, replacementName: string) {
+  private warnDeprecatedIfUsed(inputValue: any, paramName: string, fromVersion: string, replacementName?: string) {
     if (inputValue !== undefined) {
-      console.warn(`${paramName} option is deprecated since ${fromVersion}, please use ${replacementName}`)
+      if(replacementName === undefined) {
+        console.warn(`${paramName} option is deprecated since ${fromVersion}`)
+      } else {
+        console.warn(`${paramName} option is deprecated since ${fromVersion}, please use ${replacementName}`)
+      }
     }
   }
 }
diff --git a/src/Lookup/ColumnBinarySearch.ts b/src/Lookup/ColumnBinarySearch.ts
@@ -53,7 +53,7 @@ export class ColumnBinarySearch extends AdvancedFind implements ColumnSearchStra
       return rangeValue.valuesFromTopLeftCorner().map(getRawValue).map(arg =>
         (typeof arg === 'string') ? forceNormalizeString(arg) : arg
       ).indexOf(key)
-    } else if (range.height() < this.config.binarySearchThreshold || !sorted) {
+    } else if (!sorted) {
       return this.dependencyGraph.computeListOfValuesInRange(range).findIndex(arg => {
         arg = getRawValue(arg)
         arg = (typeof arg === 'string') ? forceNormalizeString(arg) : arg

diff --git a/src/Lookup/RowSearchStrategy.ts b/src/Lookup/RowSearchStrategy.ts
@@ -27,7 +27,7 @@ export class RowSearchStrategy extends AdvancedFind implements SearchStrategy {
     const range = rangeValue.range
     if(range === undefined) {
       return rangeValue.valuesFromTopLeftCorner().map(getRawValue).indexOf(key)
-    } else if (range.width() < this.config.binarySearchThreshold || !sorted) {
+    } else if (!sorted) {
       return this.dependencyGraph.computeListOfValuesInRange(range).findIndex(arg => {
           arg = getRawValue(arg)
           arg = (typeof arg === 'string') ? forceNormalizeString(arg) : arg

diff --git a/src/interpreter/binarySearch.ts b/src/interpreter/binarySearch.ts
@@ -30,7 +30,12 @@ export function rangeLowerBound(range: AbsoluteCellRange, key: RawNoErrorScalarV
     centerValueFn = (center: number) =>  getRawValue(dependencyGraph.getCellValue(simpleCellAddress(range.sheet, center, range.start.row)))
   }
 
-  return lowerBound(centerValueFn, key, start, end) - start
+  const pos = lowerBound(centerValueFn, key, start, end)
+  if(typeof centerValueFn(pos) !== typeof key) {
+    return -1
+  } else {
+    return pos - start
+  }
 }
 
 /*

diff --git a/test/config.spec.ts b/test/config.spec.ts
@@ -167,13 +167,6 @@ describe('Config', () => {
     expect(() => new Config({ undoLimit: -1 })).toThrowError('Config parameter undoLimit should be at least 0')
   })
 
-  it('#binarySearchThreshold', () => {
-    expect(() => new Config({ binarySearchThreshold: 1 })).not.toThrowError()
-    expect(() => new Config({ binarySearchThreshold: 42 })).not.toThrowError()
-    expect(() => new Config({ binarySearchThreshold: Infinity })).not.toThrowError()
-    expect(() => new Config({ binarySearchThreshold: 0 })).toThrowError('Config parameter binarySearchThreshold should be at least 1')
-  })
-
   it('#precisionEpsilon', () => {
     expect(() => new Config({ precisionEpsilon: 0 })).not.toThrowError()
     expect(() => new Config({ precisionEpsilon: 42 })).not.toThrowError()

diff --git a/test/cruds/change-cell-content.spec.ts b/test/cruds/change-cell-content.spec.ts
@@ -665,7 +665,7 @@ describe('updating column index', () => {
     const engine = HyperFormula.buildFromArray([
       ['1', '2'],
       ['3', '15'],
-    ], {binarySearchThreshold: 1, useColumnIndex: true})
+    ], {useColumnIndex: true})
 
     engine.setCellContents(adr('B2'), '8')
 
@@ -676,7 +676,7 @@ describe('updating column index', () => {
   it('should update column index when clearing cell content', () => {
     const engine = HyperFormula.buildFromArray([
       ['1', '2'],
-    ], {binarySearchThreshold: 1, useColumnIndex: true})
+    ], {useColumnIndex: true})
 
     engine.setCellContents(adr('B1'), null)
 
@@ -686,7 +686,7 @@ describe('updating column index', () => {
   it('should update column index when changing to ParsingError', () => {
     const engine = HyperFormula.buildFromArray([
       ['1', '2'],
-    ], {binarySearchThreshold: 1, useColumnIndex: true})
+    ], {useColumnIndex: true})
 
     engine.setCellContents(adr('B1'), '=SUM(')
 
@@ -696,7 +696,7 @@ describe('updating column index', () => {
   it('should update column index when changing to formula', () => {
     const engine = HyperFormula.buildFromArray([
       ['1', '2'],
-    ], {binarySearchThreshold: 1, useColumnIndex: true})
+    ], {useColumnIndex: true})
 
     engine.setCellContents(adr('B1'), '=SUM(A1)')
 

diff --git a/test/interpreter/function-hlookup.spec.ts b/test/interpreter/function-hlookup.spec.ts
@@ -90,7 +90,7 @@ describe('Function HLOOKUP', () => {
         ['1', '2', '3', '4', '5'],
         ['a', 'b', 'c', 'd', 'e'],
         ['=HLOOKUP(2, A1:E2, 2)']
-      ], {binarySearchThreshold: 1})
+      ])
 
       expect(engine.getCellValue(adr('A3'))).toEqual('b')
     })
@@ -140,7 +140,7 @@ describe('Function HLOOKUP', () => {
         ['1', '2', '3', '=TRUE()', 'foo'],
         ['a', 'b', 'c', 'd', 'e'],
         ['=HLOOKUP(TRUE(), A1:E2, 2, FALSE())'],
-      ], {binarySearchThreshold: 1})
+      ])
 
       expect(engine.getCellValue(adr('A3'))).toEqual('d')
     })
@@ -160,7 +160,7 @@ describe('Function HLOOKUP', () => {
         ['1', '2', '3'],
         ['a', 'b', 'c'],
         ['=HLOOKUP(4, A1:C2, 2, TRUE())'],
-      ], {binarySearchThreshold: 1})
+      ])
 
       expect(engine.getCellValue(adr('A3'))).toEqual('c')
     })
@@ -170,7 +170,7 @@ describe('Function HLOOKUP', () => {
         ['1', '2', '3'],
         ['a', 'b', 'c'],
         ['=HLOOKUP(0, A1:C2, 2, TRUE())'],
-      ], {binarySearchThreshold: 1})
+      ])
 
       expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.ValueNotFound))
     })
@@ -190,7 +190,7 @@ describe('Function HLOOKUP', () => {
         ['=B1', '1', '2'],
         ['a', 'b', 'c'],
         ['=HLOOKUP(1, A1:C2, 2, TRUE())'],
-      ], {binarySearchThreshold: 1})
+      ])
 
       expect(engine.getCellValue(adr('A3'))).toEqual('a')
     })
@@ -206,7 +206,7 @@ describe('Function HLOOKUP', () => {
     it('should calculate indexes properly when using binary search', () => {
       const engine = HyperFormula.buildFromArray([
         ['=HLOOKUP(4, E1:J1, 1, TRUE())', null, null, null, '1', '2', '3', '4', '5']
-      ], {useColumnIndex: false, binarySearchThreshold: 1})
+      ], {useColumnIndex: false})
 
       expect(engine.getCellValue(adr('A1'))).toEqual(4)
     })
@@ -240,6 +240,24 @@ describe('Function HLOOKUP', () => {
       expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.ValueNotFound))
     })
 
+    it('should properly report no match', () => {
+      const engine = HyperFormula.buildFromArray([
+        ['=HLOOKUP("0", A2:D2, 1)'],
+        [1, 2, 3, '\'1'],
+      ])
+
+      expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.ValueNotFound))
+    })
+
+    it('should properly report approximate matching', () => {
+      const engine = HyperFormula.buildFromArray([
+        ['=HLOOKUP("2", A2:D2, 1)'],
+        [1, 2, 3, '\'1'],
+      ])
+
+      expect(engine.getCellValue(adr('A1'))).toEqual('1')
+    })
+
     it('should coerce null to zero when using naive approach', () => {
       const engine = HyperFormula.buildFromArray([
         ['=HLOOKUP(, A2:C2, 1, FALSE())'],
@@ -255,7 +273,7 @@ describe('Function HLOOKUP', () => {
       [ '=HLOOKUP(2,2:3,2)'],
       [ 1, 2, 3],
       [ 'a', 'b', 'c'],
-    ], {binarySearchThreshold: 1})
+    ])
     expect(engine.getCellValue(adr('A1'))).toEqual('b')
   })
 
@@ -284,7 +302,7 @@ describe('Function HLOOKUP', () => {
       ['a', 'B', 'c', 'd', 'e'],
       [1, 2, 3, 4, 5],
       ['=HLOOKUP("b", A1:E2, 2)'],
-    ], {binarySearchThreshold: 1, caseSensitive: false})
+    ], {caseSensitive: false})
     expect(engine.getCellValue(adr('A3'))).toEqual(2)
   })
 })
diff --git a/test/interpreter/function-match.spec.ts b/test/interpreter/function-match.spec.ts
@@ -190,7 +190,7 @@ describe('Function MATCH', () => {
       ['300'],
       ['400'],
       ['500'],
-    ], { binarySearchThreshold: 1 })
+    ])
 
     expect(spy).not.toHaveBeenCalled()
     expect(engine.getCellValue(adr('A1'))).toEqual(4)
@@ -207,7 +207,7 @@ describe('Function MATCH', () => {
       ['300'],
       ['400'],
       ['500'],
-    ], { binarySearchThreshold: 1 })
+    ])
 
     expect(spy).toHaveBeenCalled()
     expect(engine.getCellValue(adr('A1'))).toEqual(4)
@@ -221,7 +221,7 @@ describe('Function MATCH', () => {
       ['300'],
       ['400'],
       ['500'],
-    ], { binarySearchThreshold: 1 })
+    ])
 
     expect(engine.getCellValue(adr('A1'))).toEqual(2)
   })
@@ -251,17 +251,30 @@ describe('Function MATCH', () => {
     expect(engine.getCellValue(adr('A3'))).toEqualError(detailedError(ErrorType.NA))
   })
 
-  it('should not coerce', () => {
+  it('should properly report no match', () => {
     const engine = HyperFormula.buildFromArray([
-      ['=MATCH("1", A2:A4)'],
+      ['=MATCH("0", A2:A5)'],
       [1],
       [2],
       [3],
+      ['\'1'],
     ])
 
     expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.NA, ErrorMessage.ValueNotFound))
   })
 
+  it('should properly report approximate match', () => {
+    const engine = HyperFormula.buildFromArray([
+      ['=MATCH("2", A2:A5)'],
+      [1],
+      [2],
+      [3],
+      ['\'1'],
+    ])
+
+    expect(engine.getCellValue(adr('A1'))).toEqual(4)
+  })
+
   it('should coerce null to zero when using naive approach', () => {
     const engine = HyperFormula.buildFromArray([
       ['=MATCH(, A2:A4, 0)'],