Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Batch operations #18

Closed
wojciechczerniak opened this issue Nov 17, 2019 · 6 comments
Closed

Batch operations #18

wojciechczerniak opened this issue Nov 17, 2019 · 6 comments
Labels
API Public methods and properties To Be Discussed Extra attention is needed

Comments

@wojciechczerniak
Copy link
Contributor

wojciechczerniak commented Nov 17, 2019

Description

There is no point in evaluating every CRUD operation, every data change if we know that more fields are going to change their values. Formulas evaluation should be triggered at the end of the process.

Sometimes developers want to trigger recalculation only on demand. I.e. by the user pressing a button within their application. This is similar to other spreadsheet software when the user (or developer) can switch operation mode when she expects many time-consuming worksheets.

Smart recalculation

The default operation mode of Handsontable.

batch method

By @bardek8

engine.batch(function () {
 engine.addRows();
 engine.addColumns();
});

But it would break the operations if it's not possible to apply one of them. A user wouldn't know which one was applied and which failed.

Manual calculation mode

In manual calculation mode all CRUD operations do not trigger calculations.

We can do it through API:

  • Manually trigger smart recalculate recalculate()
  • Full calculation of all the formulas recalculate(force = true)
  • Complete rebuild of the dependencies and a full calculation rebuildAndRecalculate()

And add keyboard shortcuts that are familar to our users. UI / Handsontable

This was referenced Nov 17, 2019
Closed
@wojciechczerniak wojciechczerniak mentioned this issue Dec 2, 2019
89 tasks
@wojciechczerniak wojciechczerniak added API Public methods and properties Integration labels Dec 5, 2019
@wojciechczerniak wojciechczerniak added the To Be Discussed Extra attention is needed label Jan 31, 2020
@wojciechczerniak wojciechczerniak added this to the Februrary 2020 milestone Jan 31, 2020
@wojciechczerniak
Copy link
Contributor Author

wojciechczerniak commented Feb 19, 2020

Looks like it's not finished and some TODOs left:

public batch(batchOperations: (e: IBatchExecutor) => void): ChangeList {
try {
batchOperations(this.crudOperations)
} catch (e) {
/* TODO we should be able to return error information along with changes */
}
return this.recomputeIfDependencyGraphNeedsIt()
}


It's confusing that inside batch callback we have to use some kind of batchEvaluator instead of public API. The same methods should be called (they are named the same, but they are not the same) and documented.

Also, it's not clear how nested batch operations are handled. For example: setSheetContent calls batch inside. So when I call:

engine.batch(evaluator => {
   evaluator.setSheetContent('Sheet1', [ ... ] )
   evaluator.setSheetContent('Sheet2', [ ... ] )
   evaluator.setSheetContent('Sheet3', [ ... ] )
})

the recomputeIfDependencyGraphNeedsIt will be called four times instead once? And the last call will be useless anyway.


I think we may have recalculate method already. The recomputeIfDependencyGraphNeedsIt is called after each batch callback. We can add a simple wrapper to expose it:

public recalculate() {
    this.recomputeIfDependencyGraphNeedsIt()
}

Then we have to extend it with an argument to force recalculation. And extend the API


What I've missed before is that for complex sheets there is an option to disable recalculation on change and to it on demand. To integrate such a feature we will need suspendEvaluation and resumeEvaluation methods to turn on / off the mode. And a recalculate method to calculate results on demand. Then batch could use those as well, maybe.

We were thinking about similar solution for rendering in Handsontable, so a short pseudo-code:

public batch(callback) {
   this.suspendEvaluation()
   callback()
   this.resumeEvaluation()
   this.recalculate()
}

This way we can have a toolbar with "calculate on change" toggle and "recalculate" button. Just like a full spreadsheet app has.

API required

Inspired by XL

public suspendEvaluation(): void
public resumeEvaluation(): void
public isEvaluationSuspended(): boolean
public recalculate(force?: boolean): Changes[]
public rebuildAndRecalculate(): Changes[]
public batch(callback: function): Changes[]

@wojciechczerniak
Copy link
Contributor Author

wojciechczerniak commented Mar 3, 2020

Additional config option calculationMode: auto, manual #58

@swistak35
Copy link
Contributor

@wojciechczerniak I think there are a couple of alternatives:

  1. When automatic recomputation is suspended, we don't allow to get any data from spreadsheet.
  2. We do something like "best-effort", in the middle. Sometimes we will give you old value (when you made a CRUD operation, which impact value on some formula), sometimes new one (when you had simple value in cell and changed it to another simple value -- we do that immediately, there is no "computation"), sometimes we throw an exception (when you have set a formula in some cell and it is not evaluated yet). Also, this is problematic when it comes to renaming sheets (because getters may get broken) etc.
  3. When automatic recomputation is suspended, we always return "old" value for all cells. This one is very problematic, there are two options, but both would increase memory usage significantly:
  • when calling suspendComputation, make a snapshot of all the data (cells, sheet names, named expressions etc.), and use it for all the getters. Eating a lot of memory, CPU time, and most likely it will take so much time that it was not worth it to suspend the computation anyway.
  • do the same but iteratively, but that means changing a lot of things in whole engine: making some structure which continuously store "previous" cell values, previous sheet names, previous named expressions etc.

(2) sounds annoying, (3) is big task, so maybe we can start with (1) and leave other as options for future?

@wojciechczerniak
Copy link
Contributor Author

wojciechczerniak commented Mar 17, 2020

  1. When automatic recomputation is suspended, we don't allow to get any data from spreadsheet.

Note: We've agreed to proceed with the first solution idea. The integrator can use API to get all the values and cache them. No need for us to keep everything inside the engine.

@swistak35 swistak35 mentioned this issue Mar 17, 2020
7 tasks
@swistak35
Copy link
Contributor

I think with #256 it's all here? There's an option to remove IBatchExecutor but that will be part of my changes on route to making undo redo work. I think I've done all relevant methods from proposed API, with the change that "resume" immediately recalculates (in order to not have the state where we have "resumed" computation but it is not recomputed). Resuming returns all changes, so that should be enough. And regarding rebuilding (rebuilding formulas after CRUD operations), we've agreed that it happens always and there's no option to stop it.

@wojciechczerniak
Copy link
Contributor Author

Yup, we've got everything we need. Thx 🥇


Note to future self: It's easy to implement XL style "Calculate" on-demand button. No need for public API. Just resume (aka recalculate) and suspend again so the app stays in suspended state.

function recalculate() {
  const changes = hf.resume()
  hf.suspend()
  return changes
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Public methods and properties To Be Discussed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants