Pivot grid - #4574
Draft
amcclain wants to merge 81 commits into
Draft
Conversation
…onent Brings in a generalized PivotGrid written in a client app as the starting point for a first-class hoist-react component. Committed close to as-imported, with only the parts we know are not being kept stripped out. - `PivotGridModel` builds the grid's column hierarchy from a pivot value tree and manages summary rows/columns and sorting. Removed the `debugMode` context menu, the hand-rolled `state`/`setState` persistence, the `summaryColumnConfigReaction` (redundant with `buildGridColumns`), the app-specific `right-border` / `summary-cell` styling and the `PivotGrid.scss` that disabled Hoist's own group borders, and the unused `valueColumns` getter. - `PivotDataModel` is the prototype's pivoting engine, kept for now as a reference implementation and performance baseline. It widens each leaf record with a synthetic field per (pivotPath, valueField) pair and aggregates via a `Cube`. To be replaced - `Cube` aggregation is dense over fields while the widened data is sparse, so every aggregate row computes every synthetic field. `docs/planning/pivot-grid.md` carries the plan and the review findings forward across sessions: a benchmark baseline first, then settling the pivot data API (leaning toward pivot dimensions as innermost cube dimensions with cells projected out of the row tree, though the factoring against `Query`/`View` is still open), then the data layer and `PivotGridModel` tracks in parallel. Not yet exported from any package index.
…seline Settles the vocabulary for the effort and captures the numbers the rewrite has to beat. - Terminology section: groupings / pivot dimensions / value fields, with row totals, pivot totals and value totals replacing the prototype's "summary" vocabulary, which collided with `Store.summaryRecord` and `GridModel.showSummary`. Includes a mapping table from the prototype's config names. - Acceptance criteria: seven profiles calibrated to real usage (pivots are low cardinality; the groupings carry it), each with a `+ Drill` unique-per-leaf variant. Gate is Typical - build <= 250ms, tick <= 30ms, heap <= 2x. - Baseline for `PivotDataModel` as imported, measured via the new Toolbox harness. - Phase 1 gains a pivot cardinality guard decision; phase 3 gains the API rename and pivot totals as net-new work. - Adds `cmp/pivotgrid/index.ts` so the package imports as `@xh/hoist/cmp/pivotgrid`. Headline: the prototype passes the build target and misses the tick target by 4-7x, because a tick is a full rebuild regardless of how little changed. Heap is worse than expected - 773MB on Heavy+Drill, and that is the clean figure, excluding the known per-update Cube leak. Pathological pivots (5,000 paths) degrade to 27s builds but do not crash, which argues the phase 1 guard can be a soft cap rather than a thrown error.
Design only - no implementation. The contract now lives in a "Pivot data design" section of the plan doc, which phases 2 and 3 build to. Headline: phase 0's working direction was wrong. Pivot cells cannot be extra innermost Cube dimensions - grouping is hierarchical, so a trailing pivot dimension only materializes cells beneath the *innermost* group row, leaving every group row above it with nothing to project from (and pivots outermost fails symmetrically, losing the row totals). The corrected model hangs a pivot subtree off every node of the row hierarchy, decomposed down the group axis above the innermost level and down the pivot axis below it. Those cells are real rows in the aggregation network, so every aggregator and the existing incremental `replace` machinery work unmodified, and they are never in `children` - so they never reach `getVisibleDatas` or a connected store. Decisions recorded: - Factoring is `PivotView extends View` / `PivotQuery extends Query`. Composition and fold-in rejected, with reasons. - The open question about concatenating row-group and pivot dimensions dissolved: `Query.dimensions` keeps its exact existing meaning and nothing is concatenated anywhere. - Query interface: `dimensions` plus `pivotDimensions`, `valueFields`, `emptyPathLabel`, `excludeEmptyPivotValues`, `maxPivotPaths`. - Result shape: `rows` / `leafMap` unchanged, plus a `PivotPath` tree and `cellFields`, both identity-stable while the structure holds - which is the structural-change signal. - Cells are copied onto published row data as flat synthetic fields; the leaner `getValueFn` alternative is rejected for v1 as it puts value columns outside the Store field model. - Null pivot values get their own `(empty)` segment; exclusion is expressed as a filter, the only formulation under which the totals invariant survives. - One `PivotView.PATH_DELIMITER` with injective escaping - nothing ever parses a cell field name. - `maxPivotPaths` (default 1000) throws; soft `(other)` bucketing deferred as a real top-N feature rather than a guard. - Row totals, pivot totals, and value totals are one mechanism at three depths, making pivot totals near-free for phase 3. - Correctness tests will live in the Toolbox harness - hoist-react has no test framework and `data/cube` is not standalone-importable. Also corrects two phase 0 claims: the populated cell set is *not* far smaller on every profile (for Typical it is essentially dense, and the win there is the tick, not the build), and the build targets are tightened per phase 0's own rule since the prototype passed them - Typical 250 -> 140ms, Typical+Drill 750 -> 290ms. Phase 2's row-truncation item is removed as unnecessary; phase 2 and phase 3 are now parallelizable.
Collapsed facts that had 3-5 homes down to one each, deleted the completed phase 0/1 checklists and the prototype-only findings the phase 2 rewrite resolves, and cut prose that only confirmed the design was correct. One substantive fix buried in the prose churn: baseline finding 5 argued the cardinality guard could be a soft cap or warning, contradicting phase 1's settled decision to throw. It now points at that decision instead. Kept the checkboxes and session log deliberately - a plan is short-lived scaffolding and holding status is its job, so only the untickable items came out. Every remaining box can actually be ticked.
Factors the pivot combinatorics out of PivotView into a dependency-free module over integer indices, exercised directly by `npx tsx data/cube/impl/PivotLattice.spec.ts`. The boundary is integer arrays in / integer arrays out, deliberately: `data/cube` cannot be loaded outside a bundler (the `@xh/hoist/core` barrel reaches XH and the service layer, and Hoist's `@persist` reads `descriptor.initializer`, a babel legacy-decorator shape that esbuild does not emit), so anything importing View is untestable without a browser. Indices also avoid a plan object per cell, which would fight the phase 2 heap budget at 100k+ cells. Rejects a group node carrying both leaf and group children. Such a node would give a cell two update routes into the same parent and silently double count. Reachable today via `bucketSpecFn` bucketing leaves under `includeLeaves`; PivotQuery must validate against it. The suite was mutation-tested - six deliberate defects, five caught. The sixth was an equivalent mutant, which surfaced that "C(G, rootPath) is G, never a cell" was load-bearing but unasserted; now checked directly.
…e changes to support them `View` implementation members widen to `protected`, generated root rows are retained so a subclass can walk the network, and result assembly moves behind `createResult()`. `Query.clone` now constructs via `this.constructor` with an extendable `cloneConfig`. `BaseRow` gains a second aggregation parent, `pivotParent`. Both routes share a `propagateUpdate` helper because `applyDataUpdate` rewrites each RowUpdate's oldValue/newValue in place and `Aggregator.replace` reads them - so a row with two parents must hand the second route its own copies, or it applies the first route's delta. `initAggregate` splits, exposing `initAggregateData` for cell rows: their children do not uniformly treat them as their group-axis parent, so they cannot inherit its child-parent wiring. Both take an optional field list, letting cell rows aggregate only the value fields plus `CubeFieldSpec.dependsOn`. The incremental collector becomes `Set<BaseRow>` rather than `Set<PlainObject>`, since PivotView needs the rows themselves to tell cells from group rows. PivotView's implementation fields use `declare` deliberately: initializers on a View subclass run after super()'s constructor-time fullUpdate() and would wipe everything it generated. Retires the prototype's duplicate `PivotFieldSpec.dependsOn` in favor of the new `CubeFieldSpec.dependsOn`.
A drilled-down leaf now carries its own value in its own pivot column and null elsewhere, so the row reads as a single populated column rather than a blank one. A leaf's own full-path cell is exactly its `pivotParent`, so this needs no extra bookkeeping. Hidden leaves are skipped - their data is a shared reference to Cube record data.
Populated paths move from a Set per group to sorted arrays allocated only where non-empty, and the union walk relies on a new documented precondition - parents precede their children - which removes an explicit depth computation and sort. `canAggregate` maps are now shared across every cell on a pivot path, via an optional argument to `initAggregateData` and a `computeCanAggregate` split out of it. Both were aimed at the build regression on profiles with a unique-per-leaf innermost grouping, and both are smaller wins than expected: Typical+Drill went 577 -> 562 -> 529ms against a 290ms target. The remaining cost is the ~51k cell row objects themselves (id string, data object, aggregate call). Most are innermost single-leaf-path cells whose value is just the leaf's own - not materializing those is the next thing to try.
`View.updateQuery` retains `_rowCache` for a filter-only change with simple aggregators, and a cell's cache key (`ownerRow.id` + path key + children identity) pins what its `ownerRow` and `path` *name*, not which instance names it. A cell could therefore be reused across a rebuild that minted a new owner row, projecting its values onto the discarded instance - and `pathIdx` could point at a different path entirely once a newly-admitted pivot value shifted the sorted path order. - `ownerRow` / `path` are no longer readonly; `buildCellRows` rebinds them on every build, as it already did for `parent` / `pivotParent`. - `pathIdx` is gone. `_cellFieldNames` is keyed on path identity, so a stale index is no longer representable.
`excludeEmptyPivotValues` folded into the query filter two ways that broke `Query.equals`, so `View.updateQuery`'s no-op short-circuit could never fire and the filter tree grew a node per clone. - `cloneConfig` carried `super`'s already-augmented `filter`, so each clone re-augmented it. Carry the raw config filter instead. - `FunctionFilter.equals` compares `testFn` by reference and ignores `key`, so a per-construction closure was never equal to its own clone. Use one `FieldFilter` per pivot dimension - `!= [null]`, which FieldFilter tests as null / '' / [] alike. Also makes the filter serializable; `FunctionFilter.toJSON()` throws. - `appendFilter` in place of `isEmpty(filter) ? … : [].concat(…)`, which flattens into an existing AND and stops `isEmpty` silently dropping a bare `FilterTestFn`.
Tripwire for the invariant the cell-row rebind establishes. Not reachable today - every route that could strand a `pivotParent` also rebuilds `_leafMap` empty, and the incremental path reads that first - but a new `clearCells` early return or cache-retention path in `updateQuery` would break it silently. Message is a literal rather than interpolated: `throwIf` evaluates its argument eagerly, and this runs once per cell per build.
The memo was never invalidated, so `updateQuery({valueFields})` left cell rows aggregating the previous measure set while `syncPaths` and `projectCell` read the new one - the new measure's columns projected `undefined`. Neither `_rowCache.clear()` nor `clearCells()` reached it.
Read once per build, so the memo saved one Set and one filter over `fields`. Recomputing removes the staleness class outright rather than adding an invalidation hook to remember.
Inherited as `Partial<QueryConfig>`, so `pivotDimensions` and `valueFields` failed excess-property checking from an object literal - including emptying `pivotDimensions` to degenerate to plain View behavior, which `PivotQueryConfig` documents as supported. The runtime path already worked.
`mergeSorted` documents ascending, duplicate-free inputs, but a group's own array was still the raw prefix pushes when a child merged into it. Sorting every array up front makes the precondition true, and drops a redundant re-sort - a parent was sorted again at its own iteration even though merged-in content already arrived sorted. No behavior change: the old ordering was repaired downstream by the parent's own `sortedUnique`, confirmed by mutation-testing an order-breaking `mergeSorted` fast path against the pre-change code.
`data/cube/README.md` has no pivot content - the pointers on `PivotQueryConfig` and `PivotView` were written against a section that phase 4 has yet to add.
- Broaden the mutation-test rule to both tiers, and record why the Toolbox tier has to exercise query transitions: a filter-only change with simple aggregators is the one path retaining `_rowCache`, and every bug the review found lived there. - Flag the benchmark Result table as superseded by `da632efb9`, which changed the group-axis merge on the build path. Re-measure before attacking the drill-down regression.
- Add a How to measure section. The benchmark procedure was documented against the phase 0 harness only, and the two harnesses measure different things; the trap it now names is that plain `yarn start` benchmarks the published `@xh/hoist` rather than the branch. - Record the heap gate as unverified and currently unverifiable - `PivotViewBenchModel` has no heap column, so `≤ 2×` has no runner. - Fill in what each remaining item leaves to judgment: the open number behind restating the tick gate, the routing question the drill-down fix has to settle first, which tier new aggregator coverage belongs in, the sequence that reproduces the Wide+Drill stall, a method for the plain-`View` proof, and the phase 3 dependency blocking `PivotDataModel`'s retirement.
The 2026-08-04 benchmark was taken in a hidden tab. Chrome throttles the harness's chained `await wait(50)` to 1/sec, then 1/min after five minutes, and `whileAsync` swaps to its synchronous fallback once `XH.pageIsVisible` goes false - so a backgrounded run silently inflates every figure rather than erroring. Confirmed by A/B rather than assumption: `data/cube` checked out at `7116572c6~1`, the exact code that table was measured against, reproduces HEAD's numbers and not the recorded ones. The five commits since are perf-neutral. Consequences: phase 2 clears both build gates and the delta-tick gate on every profile; the cell-row elision item is cancelled, having been justified entirely by the bad numbers; and the unexplained drill-profile tick stall was the same throttling.
Covers AverageStrict, SumStrict, Unique and ChildCount plus lenient controls, over both the aggregators' own replace semantics and full bottom-up aggregation across the lattice's CSR children. 28 -> 49 checks. Mutation-tested: 19/23 aggregator mutants and 4/4 lattice mutants killed. The four survivors are the strict aggregators' null short-circuits in replace, which are semantically equivalent - falling through re-aggregates to the same answer. What makes them safe is now asserted directly rather than left implicit. Characterizes a pre-existing SumAggregator.replace defect rather than working around it: once the last non-null constituent goes null, the incremental path lands on 0 where a rebuild reports null. Excluded from the property matrices via lenientSumZeroesOut; delete that predicate when the aggregator is fixed.
castArray(null) yields [null], and the stores = [] default at construction only covers undefined, so createView({stores: null}) threw on s.reuseRecords. null is Hoist's no-value sentinel, so passing it for "no stores" is idiomatic and should work.
RowCache.endGeneration() sweeps and logs, and ran at the end of View.generateRows() - before PivotView's override generated its cell rows. Cells land in the same cache but were created outside the generation, so they counted toward the sweep's `size` while being absent from its `live` count. Measured on Typical+Drill: 37,211 group rows + 35,000 leaves = 72,211 counted live, against a cache of 123,128 once 50,917 cells are added. That is 1.7x, past the 1.5x sweep trigger, so a steady-state pivot view swept on every single build. With the call relocated, size == live == 123,128 and it does not sweep.
The restamp added during the store-simple rebase is dead code: BaseRow.applyDataUpdate pushes an update per aggregatable field without testing whether the value moved, so every group-axis ancestor of a changed leaf - every cell owner included - is always in updatedRows and already stamped. Deleting it leaves the suite green, including 2,886 post-tick cell values read back out of a Store. Kept as insurance should that propagation become conditional, with the comment corrected to say so.
Resolves the fixed-shape vs sparse-cells decision for the incumbent: the grid layer costs ~1MB of heap and 3-9ms to build, so templating would spend dense memory to fix a cost that does not appear.
The page could not widen its tree column - no hook exists on the label column at all. Worked around with managed autosize, which is arguably the better default anyway, but it is direct evidence for the Extras call on `labelColumnOverrides`.
The first run measured PivotGridModel construction and called it the grid layer, reporting it as free. ag-Grid does not exist until a `grid()` component renders. Phase 0 had already recorded that mounting roughly doubles Typical's tick, which "free" flatly contradicts - the inconsistency was there to catch. Corrected: mount is 60-582ms, and a mounted grid multiplies tick cost by 1.4-5x, scaling with row count rather than cell count. Pivot vs a wide plain grid is unchanged - 519ms vs 489ms total build, 83.5 vs 79.7ms mounted ticks. Both conclusions drawn from the bad numbers are re-grounded rather than kept: the digest guards go because neither ever fired (retention preserved 0 of 1778 records), not because the grid is cheap; the fixed-shape decision rests on view heap, which the bug did not touch. Grid heap remains unmeasured and is now recorded as such - the deltas come out negative on 7 of 10 configs.
Resolve the agg value to `null` instead of `0` when the final non-null row becomes `null`
+ Allow fields flagged as dimensions to be used as value fields + Don't allow the same field to be used as a dimension and value field
`fields` is now purely additive on PivotQueryConfig: pivotDimensions, valueFields, and the dependsOn of those value fields are derived in, and an unspecified `fields` no longer pulls in all Cube.fields. Every aggregatable field in `fields` is aggregated on every row of the hierarchy, so inheriting the whole Cube silently paid for measures a pivot will never display. Pass `cube.fields` for the old behavior. Removed the "value field must be included in `fields`" throw, which demanded what the query can derive. cellAggFields draws solely on declared `fields`, so a narrowed set would otherwise silently drop a value field's dependencies and aggregate wrong numbers with no error. `cloneConfig` passes the raw pre-derivation fields, as it already did for the filter - re-deriving from `this.fields` would strand the fields of value fields an override is replacing, so a measure picker would accumulate dead aggregations.
Reconciles the pivot aggregation work with develop's ParentRow split: - `PivotCellRow` becomes a `ParentRow`. `ParentRow` reads its four aggregation field lists through protected getters (defaulting to the View's per-depth lists); cells override them with `PivotView`'s cell lists, since a cell has no depth. Drops our per-path shared `canAggregate` maps - develop only materializes results for fields declaring a `canAggregateFn`. - Both aggregation routes preserved: `applyDataUpdate` collects `Set<BaseRow>` and fans out via `propagateUpdate`, and `RowCache.removeParentRows` now clears `pivotParent` alongside `parent`. - `PivotQuery._rawFields` renamed to `_preAugmentFields` - develop added a private of that name to `Query`, and two same-named privates make `PivotQuery` structurally not a `Query`. CHANGELOG: our SumAggregator bullet is superseded by develop's combined Sum/Min/Max entry.
All five conflicts were our `private` -> `protected` widening against develop threading the filtered records through `filterRecords` -> `generateRows`; took both. `_recordMap` takes develop's fully-public form, which its now-lazy `AggregationContext.filteredRecords` getter needs. `PivotView.generateCells` takes the records as an argument rather than reading `_aggContext.filteredRecords`, so nothing in the framework materializes that array and develop's optimization holds for pivot views too.
+ Don't instruct to match the current file, as that tends to override all other commenting rules that are detailed below that instruction, and can propagate poor commenting practices throughout a file/package
…c keys Value column specs are expected to be shared with non-pivot grids, where a measure is often hidden by default. That `hidden: true` flowed straight through to the pivot grid, so columns arriving from a value-field change showed up hidden. `hideable: false` moves to `colDefaults`, which also keeps `GridModel.cleanColumnState` from force-hiding value columns when merging persisted state under `hideNewColumns`. `RESERVED_VALUE_COLUMN_KEYS` drives both the `PivotValueColumnSpec` type and the runtime strip, so shared specs still assign cleanly - TypeScript only rejects reserved keys written as an inline literal.
Promotes ag-Grid's `columnGroupShow` to a first-class config on both specs, making column groups collapsible without an `agOptions` escape hatch - needed for pivot grids, where `agOptions` is reserved on generated value columns. Also adds `ColumnGroupSpec.expandedByDefault`, defaulting to true: ag-Grid initializes a group's expanded state from `openByDefault`, so a newly-collapsible group would otherwise render collapsed on first paint.
Verifying in a background tab is otherwise the right way to exercise this work without taking over the desktop - but the perf panel's grid pass parks forever there, and it reads as a hang caused by whatever you just changed.
Adds `GridModel.columnGroupState`, with `isColumnGroupExpanded()`, `setColumnGroupExpanded()`, `setColumnGroupState()` and `getColumnGroups()`, persisted by default via the new `persistColumnGroups` option. Completes the framework support collapsible groups need: until now this state lived only inside ag-Grid, so it could be neither read, driven, nor saved. `setColumns` retains the state for groups the new configs still define, rather than resetting as it does for leaf column state - a pivot grid rebuilds columns whenever its data mints a new path, and that must not spring a user's collapsed groups open. `restoreDefaultsAsync` resets it explicitly. Two things worth knowing, both established against ag-Grid rather than assumed: - Its `getColumnGroupState()` also reports the padding groups it mints to balance headers, so state read back from it is normalized against our own group tree - otherwise every report would compare as a change and the write/read loop would not settle. - It keys a provided group's expanded flag on `groupId` alone, retaining it across def replacement, reorder and re-parenting. So no write-back is needed after a column rebuild, and a group that leaves the column set and returns comes back at its `expandedByDefault`.
- `PersistenceProvider.clear()` wrote through synchronously without cancelling any pending debounced write, so state returned to its default inside the debounce interval was resurrected by the stale write that followed. Affects every provider, not just the new one - verified by disabling the cancel and watching a cleared key come back holding the old value. - Persist only groups moved off their `expandedByDefault`. Its snapshot of default state is taken once at construction, so persisting the full list made a grid that later changes its group set write on every rebuild and read as dirty under a ViewManager - with no group ever toggled, which is exactly the dynamic case this feature is for. - Drop the pre-write filter against `getColumnGroupState()`: ag-Grid skips groupIds it cannot resolve rather than dropping the call, so the filter was dead code resting on a false premise, and would have silently skipped the write entirely had the col tree ever lagged the defs just applied. - Correct the `expandedByDefault` docs, which still claimed a rebuild resets to it. Left `setColumnGroupExpanded` throwing on an unknown groupId: `updateColumnState`, the targeted setter behind `setColumnVisible`, throws on an unknown colId too.
Avoids repeated conflicts with the changelog on develop across merges. Entries get re-added before the PR.
# Conflicts: # data/cube/View.ts
# Conflicts: # data/cube/View.ts # data/cube/impl/RowCache.ts # data/cube/row/ParentRow.ts
# Conflicts: # data/Store.ts # data/cube/View.ts
The row cache and `ParentRow.reuse` landed before the pivot layer and had never met it. * `reuse` read the depth-keyed agg field lists directly - a `PivotCellRow` has no depth and overrides those lists with its own, narrower set. * `reuse` claimed its children's group-axis `parent`, which corrupts both axes for a cell: a cell's children route up to it via `pivotParent`, and PivotView assigns both links from the lattice. * Cells are sparse, so a `(group, path)` pair that loses its leaves is not rebuilt and left its last value on a reused owner row - reading as a real cell and breaking `total == sum of cells`. Cells and exposed leaves now record the field names they projected, and null what a later generation no longer covers. * Projection now moves the owner row's digest, as a cell can move while the owner's own aggregates do not - leaves shifting between pivot paths, or a `canAggregateFn` keeping the owner out of an incremental update's row set entirely. Two query transitions the cache could not see, now reported by the Query itself: * A pivot dimension change re-keys every cell, and path keys carry dimension *values* alone - so a stale cell can land on a live id. Evicts unused parents, as a grouping change already did. * A `valueFields` change moves the set cells aggregate, which a gain in `Query.fields` need not reflect - a retained cell has simply never computed the added measure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.