fit: one evaluator for the fold — which was also the stall
Review of #54 found the fold reintroducing exactly the divergence the ruling exists to remove, and the same line turned out to be the 244 ms stall. One defect, two symptoms. `eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at `context.with(cat, name)`. Inside the fold `context` is a MEMBER-RECORD key carrying the none_cat coordinates, while `formula_cache` is keyed by stems with none_cats STRIPPED — so the lookup could never hit, fell through to `aggregate_raw`, found no raw cell behind a formula target, and answered Err("ref:…"). A routing formula over a derived measure gave Number(11.0) uncached and Error("ref:Margin") cached, and the cached path is the one a pivot grid runs whenever any category sits on Axis::None. `eval_expr`/`eval_bool` are hoisted to module level and BOTH evaluators now hand the fold the same record-level `eval_expr`. The two agree by construction rather than by a test. Note the repair not taken: teaching the fold to hit the cache by stripping none_cats off the member key returns the GROUP's value to every member — aggregate-then-route through a cache lookup, and silent. That same per-member `aggregate_raw` was one poorly-narrowed scan per member, an O(n^2) shape. Measured with the new `examples/fold_bench.rs`, five runs a point, at 18,050 cells: 51.1-53.3 ms before, 18.1-19.2 ms after, against 7.1-9.0 ms for a linear formula. Scaling went from superlinear (45x cells, 85x time) to linear (45x cells, 45x time). Debug 2.75 s -> 132.9-135.1 ms. The fixed point runs exactly 2 passes, measured by counting, so hoisting the member set has a 2x ceiling and is declined on improvise-9iyv — a memo keyed by less than the whole `partial` is a second answer to what a cell depends on. `Expr::Is` has an arm in each evaluator and the cached one was untested: the earlier fault was injected into the shared `eval_is`, which both arms call. Replacing the cached arm alone left the suite green. The first replacement test ALSO stayed green, which is the interesting part — once the fold routes through `eval_expr`, an `Is` inside a SUM never reaches the cached arm, so that arm is reachable only by a bare `Is` outside an aggregate. That is what the new test uses, and injecting into that arm alone now fails it. `IS` gained a word boundary: `Region ISLAND` parsed as Is(Region, "LAND"), silently wrong where it was a parse error. `is_expr` is compound-atomic with `sp+` written out, because a negative lookahead in an ordinary rule tests the character AFTER pest's auto-inserted whitespace and fails backwards. The lookahead was then redundant and removed — deleting it left the suite green, while relaxing `sp+` to `sp*` fails the test, so `sp+` is the mechanism. `WHERE` has the same hole, is far less exposed, and is filed as improvise-r08u. Also: `ambiguous.sort_unstable()` before `dedup`, which only removes consecutive duplicates, so the refusal could name a column twice. Every fault re-injected after the refactor: no-fold 10 failures, shared eval_is 5, record-level Is arm 4, cached Is arm 1, feature_roles order 2, context-vs- partial 1, the new cached-fold divergence 1, `sp+` relaxed 1. Baseline 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -496,3 +496,50 @@ than by every caller remembering to. Fault injected by restoring the `context`
|
||||
fold: this test fails with `Error("circular")` against `11.0` and the other
|
||||
eight pass, which is why it is a separate test and not an extra assertion on one
|
||||
of them.
|
||||
|
||||
### the_two_evaluators_agree_when_the_fold_reads_a_derived_measure
|
||||
|
||||
Adversarial review of PR #54, and the sharpest finding of the round: the first
|
||||
version of the fold reintroduced **exactly the divergence the ruling exists to
|
||||
remove**, through the cached path.
|
||||
|
||||
`eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at
|
||||
`context.with(cat, name)`. Inside the fold, `context` is a **member-record key**
|
||||
carrying the `none_cats` coordinates, while `formula_cache` is keyed by stems
|
||||
with `none_cats` **stripped**. The lookup could never hit, fell through to
|
||||
`aggregate_raw`, found no raw cell behind a formula target, and answered
|
||||
`Err("ref:…")`. So a routing formula reading a *derived* measure gave
|
||||
`Number(11.0)` uncached and `Error("ref:Margin")` cached — and the cached path
|
||||
is the one a pivot grid runs whenever any category sits on `Axis::None`.
|
||||
|
||||
The fixture is the four-record one plus `Margin = Cost * 2`, so the fold's input
|
||||
is a formula rather than raw data. The test asserts the uncached value **and**
|
||||
that the two agree; asserting only agreement would pass if both broke.
|
||||
|
||||
The fix makes them agree by construction rather than by this test: `eval_expr`
|
||||
is hoisted to module level and **both** evaluators hand it to `eval_agg` as
|
||||
`eval_at`. Note the repair that was *not* taken — teaching the fold to hit the
|
||||
cache by stripping `none_cats` off the member key returns the **group's** value
|
||||
to every member, which is aggregate-then-route through a cache lookup, silent.
|
||||
Fault injected by restoring `eval_expr_cached` as the fold's evaluator: this
|
||||
test fails with `Error("ref:Margin")` and the other eleven pass.
|
||||
|
||||
### the_cached_evaluator_has_its_own_working_coordinate_predicate
|
||||
|
||||
The same review's second finding, and a lesson about where to inject a fault.
|
||||
`Expr::Is` has an arm in **each** evaluator. The original evidence injected the
|
||||
fault into the shared `eval_is`, which both arms call — proving the tests see
|
||||
the predicate, not that they see the cached *copy*. Replacing only the cached
|
||||
arm with `Ok(0.0)` left the suite at 1330 passing.
|
||||
|
||||
The first attempt to close it — a coordinate split run through
|
||||
`recompute_formulas` + `evaluate_aggregated` — **also** stayed green, and that
|
||||
is the interesting part: once the fold routes through `eval_expr`, an `Is`
|
||||
inside a `SUM` never reaches the cached arm at all. The cached arm is now
|
||||
reachable only by a **bare** `Is` outside an aggregate, which is what this test
|
||||
uses. Fault injected into that arm alone: it fails, `Number(0.0)` against
|
||||
`Number(1.0)`.
|
||||
|
||||
The general shape: an injected fault proves coverage only of the code path it is
|
||||
injected into. Two arms of the same rule need two injections, and if one of them
|
||||
turns out to be unreachable, that is a fact about the design worth writing down.
|
||||
|
||||
Reference in New Issue
Block a user