Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Order of items in the array of changes after removeColumns() operation depends on the address mapping policy #1205

Closed
sequba opened this issue Mar 23, 2023 · 1 comment · Fixed by #1196 or #1242
Assignees
Labels
Bug Something isn't working Impact: Low Released

Comments

@sequba
Copy link
Contributor

sequba commented Mar 23, 2023

Description

This unit test fails:

it('removeColumns() returns the same array of changes regardless of the address mapping policy', () => {
  const data = [
    [1, 2, '=A2'],
    [3, 4, '=B1'],
  ]

  const alwaysDenseEngine = HyperFormula.buildFromArray(data, { chooseAddressMappingPolicy: new AlwaysDense() })
  const alwaysDenseChanges = alwaysDenseEngine.removeColumns(0, [0, 2])
  const alwaysSparseEngine = HyperFormula.buildFromArray(data, { chooseAddressMappingPolicy: new AlwaysSparse() })
  const alwaysSparseChanges = alwaysSparseEngine.removeColumns(0, [0, 2])

  expect(alwaysDenseChanges).toEqual(alwaysSparseChanges)
})

The root cause of it is the difference in the implementations of entriesFromColumnsSpan method in SparseStrategy and DenseStrategy.

As described by @thilgen in #1196:

AddressMapping is an abstract class that internally can use either SparseStrategy or DenseStrategy

AddressMapping.entriesFromColumnsSpan() allows the caller to request a range of cells based on a specified ColumnsSpan

Items returned from AddressMapping.entriesFromColumnsSpan() should be in the same order regardless of which strategy is in use

Unfortunately SparseStrategy returns the results by iterating first across the columns and then across the rows

  public* entriesFromColumnsSpan(columnsSpan: ColumnsSpan): IterableIterator<[SimpleCellAddress, CellVertex]> {
    for (const col of columnsSpan.columns()) {
      const colMapping = this.mapping.get(col)
      if (colMapping !== undefined) {
        for (const [row, vertex] of colMapping.entries()) {
          yield [simpleCellAddress(columnsSpan.sheet, col, row), vertex]
        }
      }
    }
  }

and DenseStrategy returns the results by iterating first across the rows and then across the columns

  public* entriesFromColumnsSpan(columnsSpan: ColumnsSpan): IterableIterator<[SimpleCellAddress, CellVertex]> {
    for (let y = 0; y < this.height; ++y) {
      for (let x = columnsSpan.columnStart; x <= columnsSpan.columnEnd; ++x) {
        const vertex = this.getCellVertex(x, y)
        if (vertex) {
          yield [simpleCellAddress(columnsSpan.sheet, x, y), vertex]
        }
      }
    }
@sequba sequba added Bug Something isn't working Impact: Low labels Mar 23, 2023
@sequba sequba linked a pull request Apr 18, 2023 that will close this issue
12 tasks
@sequba sequba changed the title Order of items returned from AddressMapping.entriesFromColumnsSpan depends on strategy Order of items in the array of changes after removeColumns() operation depends on the address mapping policy Apr 18, 2023
@sequba sequba self-assigned this Apr 18, 2023
@AMBudnik
Copy link
Contributor

Closed as done and released in HyperFormula 2.4.0

# for free to join this conversation on GitHub. Already have an account? # to comment