You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
pivot: one shared module for a PivotTable's source, headers and item labels - #4141
This is groundwork for OpenDocument Spreadsheet (ODS) support. It contains no ODS code. It moves the code that reads a PivotTable's source out of the engine and into the model, so that a future ODS reader and writer can use the same code as the engine's report.
Why ODS needs this
ODS stores a PivotTable (table:data-pilot-table) by naming things the report shows:
the source range (table:source-cell-range), as resolved from a table, a range or a defined name;
each field by its source column's name (table:source-field-name);
a page field's selected item, and each member, by its label (table:selected-page, table:data-pilot-member).
The names an ODS file holds therefore have to match the names the report computes. Otherwise a saved filter selects nothing, or a field points at a column that doesn't exist. Until now that logic lived only in gridcraft-engine (L5). A file format crate cannot depend on the engine. The layers check limits xlsx to core, formula, math and model, and an ODS reader and writer would sit at the same level. An earlier ODS draft kept its own copy of the logic, and that copy had already drifted from the engine's. Moving the logic into the model gives the engine and a file writer one copy to call.
What changes
New gridcraft_model::pivot_source:
resolve: a table without its totals row, a range (whole columns or rows cut to the used range), or a defined name, sheet-scoped or chained, followed up to 4 levels.
headers: column names, trimmed, with repeats numbered (Amount, Amount2). An empty header is an error. Each style's format is parsed once.
column_format, date_group_item and item_label.
ItemFormat: an opaque, pre-parsed number format for item labels. A crate without a numfmt dependency, such as a file format crate, can build one from a code string and call item_label. A code too long to be valid is General without being scanned, as the grid shows it.
Engine (pivot.rs, pivot/items.rs, pivot/compute.rs): calls these functions instead of its own copies. The report parses each filter, row and column field's item format once per report instead of once per record. The page-field grouping lookup uses the new PivotTable::axis_field.
numfmt::Formatted::plain: the formatted text with a repeat fill shown as one space. engine::display::value_text_as and the pivot headers use it.
New dependency:gridcraft-model now depends on gridcraft-numfmt (L2 → L0, which cargo xtask layers allows). gridcraft-xlsx reaches numfmt only transitively, and the FormatDep rule (no direct xlsx → numfmt dependency) still holds. This is for the reviewer to decide on.
docs/architecture.md: the model row, timestamp line and revision history are updated.
User-visible behaviour
One fix: column headers are read from the cell's value, not its =formula text, while Show Formulas is on. On main, a formula header with Show Formulas on became a field named after the formula. Test: headers_read_values_while_formulas_show.
Item labels are unchanged. Number items keep the text they had on main, without the repeat fill. Under an accounting format a label stays " $120 ", so stored selected and collapsed_items labels and subtotal captions still match. Test: accounting_items_keep_their_labels_without_the_fill, which fails if the label goes through plain().
Nested arrays: an array inside an array is (blank) as an item, and no header name, with no recursion. Nothing that calc produces should reach this. Tests: array_items_agree_in_value_key_and_label and the model unit tests.
Some tests only pin behaviour main already has, so that the shared module keeps it: table_source_leaves_out_its_totals_row, sheet_scoped_and_chained_names_resolve, repeated_and_padded_headers_keep_their_fields and formatted_page_items_are_chosen_by_their_labels.
Testing
Run locally on macOS (aarch64) after merging current main:
cargo test --workspace --lib --bins --tests --no-fail-fast: everything passes except two tests that this PR does not touch:
gridcraft-ui-egui --test arabic_editingcaret_after_typing_arabic_is_drawn_at_its_left_end also fails on an unmodified main checkout on this machine.
gridcraft-math --test gatepathological_source_at_the_cap_finishes_in_bounded_time exceeds its wall-clock budget when the machine is under heavy load. It passes when run on its own.
The ui-egui tests were run with CRAFT_FONTS_DIR set to the craft-fonts commit that CI pins. Without it, two tests fail for lack of glyphs, as they do on main.
cargo test --workspace --doc passes.
Review rounds
The branch went through three rounds of review, with fixes, before this PR:
Round 1:
item_label took a numfmt::NumberFormat, which a file format crate can't name. It now takes ItemFormat.
Long format codes were parsed uncached on every record. A 2 MB code made a 20,000-record report take 37 s.
The header test couldn't tell plain() from text.
Nested-array headers disagreed with items.
Round 2:
The round 1 fix still looked each record's format up in the cache. Formats are now parsed once per field.
A misleading comment was corrected.
A test now covers nested-array headers.
Round 3: no defects.
These were considered and left as they are:
headers has no byte limit. That limit predates this PR, since the engine caps cells, not bytes.
The rule that keeps items apart (text compared case-insensitively, numbers by value) stays in the engine. A writer that names selected items by label doesn't need it. The ODS PR can move it if it needs to list every member.
Overlap
#1684 changes whether source headers are trimmed. Whichever PR lands second should apply that change in pivot_source::headers.
Left out
All ODS code.
ItemLabels, which the ODS PR adds.
filter_button, which is in another split.
The engine's other copies of the axis-field lookup in cmd/pivot/*.
Summary by CodeRabbit
Bug Fixes
Improved PivotTable handling of table and range sources, including totals rows, defined names, and whole-row or whole-column selections.
Improved header recognition for blank, padded, duplicate, and formula-based headers.
Updated formatted labels and filters to better reflect number formats, date groupings, and values in array cells.
Improved display of repeat-fill formatting in plain text.
…labels
model::pivot_source resolves a PivotTable's source (a table without its
totals row, a range, or a defined name, sheet-scoped or chained), names its
columns (trimmed, repeats numbered) and labels its items (column format, date
groups, (blank)). The engine's reports call it instead of their own copies, so
file writers can state the same columns and items. PivotTable::axis_field
replaces the engine's page-field grouping lookup, and numfmt's
Formatted::plain replaces the display code's inline fill handling.
Column headers are now read as values even while Show Formulas is on, and a
repeat-fill in a number format shows as one space in item labels.
Number item labels keep the report's text with a repeat fill left out (as
before this branch), so stored page selections such as " $120 " under an
accounting format still match. item_label reads an array's first element once
and calls a nested array (blank), as the item's value and key do, without
recursing. MONTHS is private, and ItemLabels is left out until the file writer
that uses it lands.
…unscanned
Round 1 review fixes for the shared pivot source module.
- item_label took a numfmt::NumberFormat, which a file writer in the xlsx
crate cannot name (the layers check bars xlsx -> numfmt). It now takes
pivot_source::ItemFormat, built from a code string.
- make_item parsed the column format uncached for every record (text and
blank ones too), so a multi-megabyte code from a file made a report take
seconds per thousand records. It uses the engine's format cache again, and
a code over 4 x the character limit is General without being copied or
scanned, as the grid shows it.
- headers parse each style's format once, not once per column, with the
same cap.
- A header holding an array inside an array is read one level deep, as an
item is, instead of formatting the inner value.
- The header unit test used a trailing fill that trims away, so it could not
tell plain() from text; it now uses a leading one.
- Formatted::plain's doc said labels use it; item labels keep text.
Round 2 review fixes, correcting round 1's make_item change.
- Round 1 had make_item look its column's format up in the format cache
for every record: a global lock and a hash of the code per record, text
and blank ones included, which main never paid for them. The report now
builds each filter, row and column field's ItemFormat once, and
distinct_labels does the same for its column; FieldItems::intern and
make_item take the parsed format.
- item_label's doc says callers pass each record as its cell's first
element, as the report does; the engine comment said the opposite.
- A test pins that a header is read one array level deep, as items are.
No actionable comments were generated in the recent review.
Walkthrough
The model now provides shared PivotTable source resolution, header extraction, and item-label helpers. The engine uses these helpers in pivot processing. The number-format crate adds plain-text output for repeat-fill formats, and tests cover source handling and formatted labels.
The model adds resolution for table, range, and defined-name sources, plus formatted and deduplicated header extraction. The engine delegates source handling to these helpers. Tests cover totals rows, defined names, duplicate headers, and formula-valued headers.
Format pivot items and report filters crates/model/src/pivot_source.rs, crates/model/src/features/pivot.rs, crates/engine/src/pivot/compute.rs, crates/engine/src/pivot/items.rs, crates/engine/src/pivot.rs, crates/engine/src/cmd/pivot/tests.rs
The model adds formatted item labels and date-group keys. The engine parses item formats for aggregation and uses axis_field to find date-group information. Tests cover array values and formatted filter labels.
Return plain text for repeat-fill formats crates/numfmt/src/lib.rs, crates/numfmt/src/tests.rs, crates/engine/src/display.rs, crates/engine/src/cmd/pivot/tests.rs
Formatted::plain() inserts a space at a valid repeat-fill position. Engine display uses this method, and tests cover repeat-fill output and accounting-format filter labels.
This refactor moves PivotTable source resolution and item-label logic into a shared model module without intended behavior changes. No concrete defect remains, so the risk of merging is minimal.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names a shared module for PivotTable source, headers, and item labels. The changeset moves source resolution, header naming, and item-label logic into the new gridcraft_model::pivot_source m…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names a shared module for PivotTable source, headers, and item labels. The changeset moves source resolution, header naming, and item-label logic into the new gridcraft_model::pivot_source module, so the title matches the main change. The title uses the prefix 'pivot:' and is 12 words, which is slightly long but still clear.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between c0ee120 and 2891981.
Files ignored due to path filters: 1
Cargo.lock is excluded by !**/*.lock
Files selected for processing: 12
crates/engine/src/cmd/pivot/tests.rs
crates/engine/src/display.rs
crates/engine/src/pivot.rs
crates/engine/src/pivot/compute.rs
crates/engine/src/pivot/items.rs
crates/model/Cargo.toml
crates/model/src/features/pivot.rs
crates/model/src/lib.rs
crates/model/src/pivot_source.rs
crates/numfmt/src/lib.rs
crates/numfmt/src/tests.rs
docs/architecture.md
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
This branch has not been deployed
No deployments
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
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.
This is groundwork for OpenDocument Spreadsheet (ODS) support. It contains no ODS code. It moves the code that reads a PivotTable's source out of the engine and into the model, so that a future ODS reader and writer can use the same code as the engine's report.
Why ODS needs this
ODS stores a PivotTable (
table:data-pilot-table) by naming things the report shows:table:source-cell-range), as resolved from a table, a range or a defined name;table:source-field-name);table:selected-page,table:data-pilot-member).The names an ODS file holds therefore have to match the names the report computes. Otherwise a saved filter selects nothing, or a field points at a column that doesn't exist. Until now that logic lived only in
gridcraft-engine(L5). A file format crate cannot depend on the engine. The layers check limitsxlsxtocore,formula,mathandmodel, and an ODS reader and writer would sit at the same level. An earlier ODS draft kept its own copy of the logic, and that copy had already drifted from the engine's. Moving the logic into the model gives the engine and a file writer one copy to call.What changes
gridcraft_model::pivot_source:resolve: a table without its totals row, a range (whole columns or rows cut to the used range), or a defined name, sheet-scoped or chained, followed up to 4 levels.headers: column names, trimmed, with repeats numbered (Amount,Amount2). An empty header is an error. Each style's format is parsed once.column_format,date_group_itemanditem_label.ItemFormat: an opaque, pre-parsed number format for item labels. A crate without anumfmtdependency, such as a file format crate, can build one from a code string and callitem_label. A code too long to be valid is General without being scanned, as the grid shows it.pivot.rs,pivot/items.rs,pivot/compute.rs): calls these functions instead of its own copies. The report parses each filter, row and column field's item format once per report instead of once per record. The page-field grouping lookup uses the newPivotTable::axis_field.numfmt::Formatted::plain: the formatted text with a repeat fill shown as one space.engine::display::value_text_asand the pivot headers use it.gridcraft-modelnow depends ongridcraft-numfmt(L2 → L0, whichcargo xtask layersallows).gridcraft-xlsxreaches numfmt only transitively, and the FormatDep rule (no direct xlsx → numfmt dependency) still holds. This is for the reviewer to decide on.docs/architecture.md: the model row, timestamp line and revision history are updated.User-visible behaviour
=formulatext, while Show Formulas is on. Onmain, a formula header with Show Formulas on became a field named after the formula. Test:headers_read_values_while_formulas_show.main, without the repeat fill. Under an accounting format a label stays" $120 ", so storedselectedandcollapsed_itemslabels and subtotal captions still match. Test:accounting_items_keep_their_labels_without_the_fill, which fails if the label goes throughplain().(blank)as an item, and no header name, with no recursion. Nothing that calc produces should reach this. Tests:array_items_agree_in_value_key_and_labeland the model unit tests.mainalready has, so that the shared module keeps it:table_source_leaves_out_its_totals_row,sheet_scoped_and_chained_names_resolve,repeated_and_padded_headers_keep_their_fieldsandformatted_page_items_are_chosen_by_their_labels.Testing
Run locally on macOS (aarch64) after merging current
main:cargo xtask ci: fmt, clippy (-D warnings), i18n-report, assets, layers, wasm, parity and keytips pass.cargo test --workspace --lib --bins --tests --no-fail-fast: everything passes except two tests that this PR does not touch:gridcraft-ui-egui --test arabic_editingcaret_after_typing_arabic_is_drawn_at_its_left_endalso fails on an unmodifiedmaincheckout on this machine.gridcraft-math --test gatepathological_source_at_the_cap_finishes_in_bounded_timeexceeds its wall-clock budget when the machine is under heavy load. It passes when run on its own.CRAFT_FONTS_DIRset to the craft-fonts commit that CI pins. Without it, two tests fail for lack of glyphs, as they do onmain.cargo test --workspace --docpasses.Review rounds
The branch went through three rounds of review, with fixes, before this PR:
item_labeltook anumfmt::NumberFormat, which a file format crate can't name. It now takesItemFormat.plain()fromtext.These were considered and left as they are:
headershas no byte limit. That limit predates this PR, since the engine caps cells, not bytes.Overlap
#1684 changes whether source headers are trimmed. Whichever PR lands second should apply that change in
pivot_source::headers.Left out
ItemLabels, which the ODS PR adds.filter_button, which is in another split.cmd/pivot/*.Summary by CodeRabbit