feat(config): add arrayFunctionResultOverwritesData opt-in overwrite flag (HF-305)#1714
feat(config): add arrayFunctionResultOverwritesData opt-in overwrite flag (HF-305)#1714marcin-kordas-hoc wants to merge 1 commit into
Conversation
✅ Deploy Preview for hyperformula-dev-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
9009463 to
b2da92e
Compare
Performance comparison of head (467c186) vs base (23a7437) |
|
bugbot run |
4c02791 to
d2a2671
Compare
|
bugbot run |
d2a2671 to
5e397d1
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e397d1. Configure here.
5e397d1 to
2f48fba
Compare
2f48fba to
b877ed4
Compare
|
bugbot run |
…flag (HF-305) Adds a public boolean Config option `arrayFunctionResultOverwritesData` (default `false`). When `false`, behavior is unchanged: an array spill onto an occupied cell yields `#SPILL!` and leaves the occupant intact. When `true`, the spill overwrites the occupied cells (clears occupants, spills the array, reroutes dependents) instead of emitting `#SPILL!`. Implementation reuses the existing spill-placement exchange primitive: - ConfigParams/Config: new option, mirrored on `useArrayArithmetic`. - DependencyGraph gains `config` + two guarded early-returns in exchangeOrAddFormulaVertex / setAddressMappingForArrayVertex. - Evaluator: recompute no longer emits `#SPILL!` when overwrite is allowed. Array-vs-array safety: a spill colliding with ANOTHER array always keeps `#SPILL!` and leaves that array intact, even in overwrite mode (canOverwriteArrayResult / overwriteWouldHitArray). This matches Excel and avoids corrupting a pre-existing array; overwrite only clears static data. Default `false` guarantees no existing embedder loses data silently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b877ed4 to
467c186
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 467c186. Configure here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1714 +/- ##
===========================================
- Coverage 97.21% 97.21% -0.01%
===========================================
Files 178 178
Lines 15611 15660 +49
Branches 3460 3441 -19
===========================================
+ Hits 15177 15224 +47
+ Misses 434 428 -6
- Partials 0 8 +8
🚀 New features to boost your workflow:
|
What & why
Adds an opt-in boolean config option
arrayFunctionResultOverwritesData(defaultfalse).false(default): unchanged behavior — an array function spilling onto an occupied cell yields#SPILL!and leaves the occupant intact.true: the spill overwrites the occupied cells (clears occupants, spills the array, reroutes dependents) instead of emitting#SPILL!.Requested by a customer; the feature + option name were pre-agreed (~8 SP).
How
Reuses the existing spill-placement exchange primitive rather than inventing a mechanism:
ConfigParams/Config: new option, mirrored onuseArrayArithmetic.DependencyGraphgainsconfig+ two guarded early-returns (exchangeOrAddFormulaVertex,setAddressMappingForArrayVertex).Evaluator: recompute no longer emits#SPILL!when overwrite is allowed.Array-vs-array safety: a spill colliding with another array always keeps
#SPILL!and leaves that array intact, even in overwrite mode (canOverwriteArrayResult/overwriteWouldHitArray). Matches Excel; overwrite only clears static data. Defaultfalseguarantees no silent data loss.Tests
test/hf-305-overwrite.spec.ts(OFF parity →#SPILL!+ occupant intact; ON overwrite + dependent reroute + recalc; array-vs-array stays#SPILL!). Full array/spill + undo-redo regression green.Known limitations
The opt-in overwrite behavior is intentionally scoped to the direct spill/recalc path. The following edges are not fully handled yet; each is a nuance to weigh, not a silent-corruption risk (the default
falseconfig is fully backward compatible and untouched by all of these):buildFromArrayinitial-build path: declaring an array formula and a conflicting occupant in the same initialbuildFromArraycall still lets the occupant win — overwrite only applies on the recalc /setCellContentspath (existing data, then an array formula lands on it), not the un-gated initial-build placement path.setCellContents(verified):undo()removes the array formula but the overwritten occupants are lost, not restored — they are not recorded on the undo stack. Documented in theConfigParamsJSDoc; a proper fix (recording cleared occupants in undooldData) is follow-up work. Top item for reviewer to weigh given the feature is destructive.addColumns/addRows/removeColumns/removeRows) that grows an array over static data does not snapshot for undo either. When a structural op shifts a dependency so an existing array grows and overwrites a previously-static cell, the overwrite itself is now handled correctly for evaluation and foruseColumnIndex(the stale value is dropped from the column index at the moment of overwrite — fixed during review, seerewriteAffectedArrays), but undoing that structural op does not restore the static cell's original value or its column-index entry. This is the same undo gap as the point above, on the structural-op path instead of the direct-edit path.useColumnIndex+ undo interaction: because of the two points above, any workflow that relies onundo()to fully roll back an overwrite (whether triggered directly or by a structural op) will see the array formula disappear but the previously-static data stay gone, and (ifuseColumnIndexis on) MATCH/VLOOKUP/HLOOKUP will correctly not find the old value post-undo, but will also not find it restored — i.e. the index and the data are consistent with each other post-undo, just not consistent with pre-overwrite state.None of these limitations can produce the crash or stale-lookup-index bugs originally flagged by Bugbot on this PR — both were reproduced with throwaway probes and fixed at the root cause (see review thread replies on commit
b877ed4). What remains out of scope is exclusively the undo leg of overwrite bookkeeping.Note
High Risk
Opt-in destructive sheet mutation in core dependency graph, evaluation, undo, and column-index paths; mistakes could silently clear data or leave stale lookup indexes despite broad new tests.
Overview
Adds opt-in config
arrayFunctionResultOverwritesData(defaultfalse). Whentrue, array formulas that would#SPILL!onto non-empty cells instead claim the spill range, clear static occupants, and evaluate—array-vs-array collisions still#SPILL!.Engine:
DependencyGraphtakesConfig, exposescanOverwriteArrayResult/overwriteWouldHitArray, and extends array placement inexchangeOrAddFormulaVertexto allow overwrite and emit content changes for cleared cells (using non-throwingvalueOrUndeffor uncomputed formula occupants).Evaluatorskips#SPILL!when overwrite is allowed.Operations / undo:
setCellContentreturns{ oldContent, overwrittenCells }via pre-spill snapshots;SetCellContentsUndoEntrystores overwritten cells andundo()restores them after the anchor formula is reverted. Column index stays consistent on overwrite, array expand/shrink restore, and selectiverewriteAffectedArraysupdates when structural ops grow arrays over static cells (useColumnIndex).Tests:
test/hf-305-overwrite.spec.tscovers default vs explicit flag, dependent reroute, undo/redo, array-array guard, batch uncomputed occupant, and column-index staleness (includingaddColumnsgrowth).Reviewed by Cursor Bugbot for commit 467c186. Bugbot is set up for automated code reviews on this repo. Configure here.