fix: resolve a cell's format through the formula that owns it
Both findings from ChatGPT Codex's review of PR #125. Both are real, and both are the same mistake in two places: answering "whose format is this" from something other than the rule that answers "whose value is this". **1. `FormatPlan` indexed formats in a `HashMap` keyed by target.** A target category need not be `_Measure` — `[CustomCat]` is in the format's own documentation — so a key can carry coordinates two formulas claim, and iterating the map answered from whichever category hashed first: the displayed format was nondeterministic per process while the value was not. That is gotcha 18's shape. Worse, only *formatted* formulas were indexed, so a later formula's `FORMAT` reached a cell whose value an earlier unformatted formula produces. `FormatPlan` now holds every formula's `(target_category, target, Option<format>)` in the model's own order and takes the first coordinate match — `Model::formula_for`'s rule and order exactly. A match naming no format falls through to the view; it is a match, not a miss. The list is dropped entirely when no formula names a format, so the common document still pays one `is_empty` per cell. **2. A column total took its format from row 0.** `context/code/core.md` argued that was safe because "where `_Measure` is on the row axis a column is not a measure at all". Wrong: a formula's target category is any category, so a formula can own a row-axis item — the total then inherited a row-local override for a row it had summed away, and re-ordering the rows changed its spelling. New `GridLayout::column_key(col)` gives the coordinates that survive the row aggregation (page/filter constraints plus the column's tuple), and `col_formats` is resolved from that, once per column, outside the row loop. Three regression tests, each with the rejected version injected as the fault: - `the_formula_that_owns_a_cell_names_its_format_even_when_it_names_none` puts the same two formulas in both orders, so preferring the formatted one fails one half and ignoring formats fails the other. - `the_owner_is_the_one_model_order_names_and_not_the_one_a_hash_does` rebuilds the plan five times, since each `HashMap` gets its own `RandomState`. - `a_column_total_does_not_inherit_a_row_local_format` builds one grid with the owned row first and one with it last and requires the strips to agree. Every raw cell holds the same value per measure, so the totals are numerically identical and only the format can differ. `context/code/core.md`'s wrong paragraph is replaced rather than amended, and says what it got wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
+33
-8
@@ -870,15 +870,40 @@ which falls out of the key rather than needing a rule. Deriving it from "is
|
||||
with itself while disagreeing with the cells.
|
||||
|
||||
**The empty plan short-circuits**, so a document with no formula format pays one
|
||||
`is_empty` per cell and nothing else; the lookup is otherwise one hash get per
|
||||
`(category, item)` pair in the plan, of which there is normally one.
|
||||
`is_empty` per cell and nothing else; the lookup is otherwise a scan of the
|
||||
formula list, which is short and must stay ordered for the reason below.
|
||||
|
||||
**A column total takes the format of its own column's first row.** `GridBody`
|
||||
carries `col_formats` beside `col_widths` so the totals strip and the cells
|
||||
above it cannot disagree. Where `_Measure` is on the *row* axis a column is not
|
||||
a measure at all, so no formula format applies to any cell in it and the first
|
||||
row's answer is the view's — the rule only has to be decided for the case it
|
||||
cannot arise in.
|
||||
**The owner is the formula `Model::formula_for` names, and nothing else.**
|
||||
`FormatPlan` holds every formula's `(target_category, target, Option<format>)`
|
||||
in the model's own order and takes the **first** coordinate match, which is the
|
||||
same rule and the same order evaluation uses to decide whose value the cell is.
|
||||
Two things that shape enforces, both of which a `HashMap` keyed by target got
|
||||
wrong and review caught (Codex, PR #125):
|
||||
|
||||
- **A key may be claimed by two formulas**, because a target category need not
|
||||
be `_Measure` — `- Tax = Revenue * 0.1 [CustomCat]` is in the format's own
|
||||
documentation. Iterating a `HashMap` answered from whichever category hashed
|
||||
first, so the *displayed* format was nondeterministic per process while the
|
||||
*value* was not. That is gotcha 18 exactly: an index whose values are hashed
|
||||
is for lookup, never for choosing between candidates.
|
||||
- **An owner that names no format still owns the cell.** Indexing only the
|
||||
formatted formulas let a later formula's `FORMAT` reach a cell whose value it
|
||||
does not produce. So the list holds *every* formula and a match with
|
||||
`format: None` falls through to the view — it is a match, not a miss.
|
||||
|
||||
The list is dropped entirely when no formula names a format, so the common
|
||||
document pays one `is_empty` per cell rather than a scan.
|
||||
|
||||
**A column total is resolved from the column, never from a row.**
|
||||
`GridLayout::column_key(col)` is the page/filter constraints plus the column's
|
||||
own tuple — the coordinates that survive the row aggregation — and `GridBody`
|
||||
carries the resulting `col_formats` so the strip and the cells cannot disagree.
|
||||
Reading it off row 0 was the first version and is wrong twice over, also found
|
||||
by review: a formula may target a **row-axis** item (its target category is any
|
||||
category), so the total inherited a row-local override for a row it had summed
|
||||
away, **and** re-ordering the rows changed the total's spelling. The guard is
|
||||
`a_column_total_does_not_inherit_a_row_local_format`, which builds the same grid
|
||||
with the owned row first and last and requires the two strips to agree.
|
||||
|
||||
### The grammar: `FORMAT <spec>` in the formula, not in the `.improv` line
|
||||
|
||||
|
||||
Reference in New Issue
Block a user