Repository navigation
Conversation
Contributor
Author
|
Merge order for my open PRs. Some of them touch the same files, so this order avoids conflicts: Independent (any order, each merges cleanly on Linked workbooks: #417, then #464. They conflict in PivotTables, in this order:
Drawings: #685 (grouped shapes), then #719 (slicers). Every pair that overlaps has been merged and tested together locally (fmt, clippy, xlsx/engine/model tests, and a round-trip check on 11 real reports with 0 differences). Each PR's description explains how its conflict resolves. Whichever PR of a pair lands second, I'll rebase it the same day. Edited 2026-10-14: #464 moved out of the independent list; it now conflicts with #417. |
…yout settings A refreshed PivotTable read from an Excel file now lays out like Excel's: - Sort by a value field (autoSortScope on data field n): PivotField.sort_by, applied within each parent; pivot.sort takes `by: "Sum of X"`. - Captions: rowHeaderCaption, colHeaderCaption, dataCaption, grandTotalCaption and missingCaption (For empty cells show), field custom names and renamed items (item n). - Manual item order of fields that aren't auto-sorted. - Per-field subtotals (defaultSubtotal, with the table default in pivotTableDefinition16), Repeat Item Labels (fillDownLabels), field number formats, the Values row (hideValuesRow) and the classic layout (gridDropZones). - rowGrandTotals/colGrandTotals were read and written the wrong way round. - Date items sort after text, as in Excel. All settings are written back to XLSX. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Sort by the values in a column (autoSortScope references naming items of the other axis): read, written back and used by the engine, instead of sorting by the grand total. A scope GridCraft can't represent loads as a sort by label, with a warning. - Changing subtotals in Design replaces each field's own subtotal setting. - A numeric "For empty cells show" caption (Excel stores missingCaption="0") is written as a number in the value's format. - With field headers off there's no Values row, so firstDataRow matches. - A field that isn't auto-sorted keeps its <items> order even when it is cache order (date groups only when reordered). Items tied in a sort by value still break ties by label in the sort direction: over 60 tie groups in the 11 Excel-saved reports no stored order (pivotField items, sharedItems, first appearance, reversed or not) explains more than 21, the same as the current rule.
…te order of date groups - Sort scope entries name their field by source column and date grouping (PivotScopeItem), so Years and Quarters of one date column stay apart when reading, computing and writing. - The engine and the writer resolve a scope the same way: a scope naming an item that's no longer there sorts by the grand total, as it is then saved. - A field with custom subtotal functions (sumSubtotal...varPSubtotal) has subtotals on; it shows the automatic subtotal, with a warning. - Date groups that aren't sorted show in date order, the order of the cache's group items, instead of source order.
…s subtotal default on
neilcawse
force-pushed
the
fix/pivot-refresh-fidelity
branch
from
October 13, 2026 18:27
7502701 to
add4ebf
Compare
added 7 commits
October 14, 2026 09:11
…items Excel writes a pivot cache's date items as ISO dates counting real days from 1899-12-30: a time is 1899-12-30T08:00:00 and serial 1 is 1899-12-31. GridCraft read them with DATE() rules, which move year 1899 on to 3799, so those items never matched the report's items. A refresh then put them after every item that did match, such as (blank), and saving wrote that wrong order into the file. ISO dates (pivot cache items, t="d" cells) now go through a calendar-date conversion that counts real days from the epoch, accepts 1900-02-29 as serial 60, and in the 1904 system takes 1899-12-30 as a time's date part. The cache writer uses the inverse, so serial 1 is written as 1899-12-31 as Excel does. DATE() keeps its own rules.
The ascending item order put dates between text and TRUE/FALSE. Excel's item lists put numbers, text, logicals, errors, dates and then blanks: in the one Excel-saved cache in the test corpus whose fields mix dates with logicals or errors (ClosedXML VariousDataTypesInTableColumns), the Mixed Types field lists 10, 14.5, ABC, TRUE, #DIV/0!, 16:30, 8/10/2020, (blank), and the DateTime with num field puts its numbers before its dates. The sort classes follow that order. Also adds the regression test for the time items read from an Excel cache.
The previous commit changed the shared ISO date parser, so worksheet t="d" cells dated before March 1900 also read one day later. The evidence for the real-day convention comes from Excel's pivot caches only, so worksheet cells read exactly as before again. The parser takes the date conversion as a parameter: worksheet cells keep DATE() rules, pivot cache items use the real-day conversion that the cache writer mirrors.
The date sort class came from the number format of the column's first data cell. Excel decides per cell (it caches date-formatted cells as <d> and General ones as <n>), so a General first cell made every date sort as a number, and a date-formatted first cell made General numbers sort as dates. The source now records, per cell, whether its own format is a date, and the class follows that.
Reading every source cell's style made each refresh of a large source much slower. The source now keeps the sheet row of each record, and the per-cell date flags are read only for ungrouped row and column fields, one format lookup per distinct style. One number in a date cell and in a General cell is one item; its sort class depended on which cell came first. Any General occurrence now makes it a number.
… number The General-wins rule ran in item interning, which only sees records that pass the report filter, so filtering out the General cell made its number sort as a date. The date flags are now decided over every record of the column before filtering, as Excel's cache decides.
date_cells hashed every non-date number of a column into the General set even when no cell of the column was a date, which a numeric General row field paid for on every refresh. The set is now built only when some flag is set.
This branch has not been deployed
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
1 participant
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.
Refreshing a PivotTable read from an Excel file gave a different report from the one Excel shows. Items came out sorted by label when the file sorts them by a value, headers read "Row Labels", "Values" and the source column names instead of the file's captions, renamed items lost their names, manually ordered columns were re-sorted, and per-field subtotal and label settings were dropped. The PivotTable definition stores all of these settings, but GridCraft didn't read them, or read them and didn't apply them.
What changed
The model changes are in
crates/model/src/features.rs, the layout changes incrates/engine/src/pivot.rsand the XLSX read/write incrates/xlsx/src/pivot.rs. Every new field is#[serde(default)].PivotField.sort_by: Option<String>(a value field's caption) sorts the items by that value field in thesortdirection, within each parent item, with subtotals. It is read from<autoSortScope>when the scope refers to the Values field (field="4294967294") and data item n, and written back the same way.pivot.sorttakesby: "Sum of Sales"(a caption or a source field in the Values area). Renaming the value field withpivot.valueSettingskeeps the sort.PivotField.sort_scope: Vec<PivotScopeItem>(source column, date grouping, item label) says where the sort takes its values: Excel's "sort by the values in the Apples column" adds a reference to an item of a field on the other axis to the<autoSortScope>. It is read, written back (as the index into that field's<items>) and used by the engine, which used to sort by the grand total. Entries name their field by source column and date grouping, so Years and Quarters grouped from one date column stay apart. The engine and the writer resolve a scope the same way: the other axis's fields outer first, up to the first one it doesn't name; a scope naming an item that's no longer there sorts by the grand total, and is saved as a sort by the grand total. A scope GridCraft can't represent (a reference with no item or with several,byPosition,relative, or a field that isn't on the other axis) loads as a sort by label, with a warning.pivot.sortclears the scope.PivotTable:row_header_caption,col_header_caption(Compact headers),data_caption(the Values field,dataCaption; it used to be written but not read),grand_total_caption(grandTotalCaption) andempty_caption(For empty cells show:missingCaptionwithshowMissing). A caption that reads as a number (Excel storesmissingCaption="0") is shown as a number, in the value field's format. OnPivotField:caption(custom name,<pivotField name>, shown in headers and in the "X Total" subtotal labels) anditem_captions(renamed items,<item n>).PivotField.item_orderkeeps the order of a field's<items>when it isn't auto-sorted, even when that order happens to be cache order, so a refresh over reordered source rows keeps it. Cache items name the report's items by value: a<d>item counts real days from 1899-12-30 the way Excel writes them, so a time (1899-12-30T08:00:00) or a date before March 1900 (serial 1 is1899-12-31) names the right item instead of falling behind(blank). GridCraft writes those dates the same way. This applies to pivot caches only; worksheett="d"cells read as before. Items the list doesn't name follow in source order. Date groups that aren't sorted show in date order (the order of Excel's cache group items; they used to come out in source order), so they keep a list only when it was reordered, and unlisted groups follow in date order.PivotField.subtotal: Option<bool>(defaultSubtotalper field; the table default goes toxpdl:pivotTableDefinition16 EnabledSubtotalsDefaultwhen fields override it),repeat_labels(Repeat Item Labels,x14:pivotField fillDownLabels),number_format(field number format for item labels,numFmtId). OnPivotTable:show_values_row(x14:pivotTableDefinition hideValuesRow) andclassic_layout(gridDropZones: with one value field and no column field, the value caption goes above the row headers and the value column is headed "Total"). Changing subtotals in Design (pivot.layout) now replaces each field's own subtotal setting, as Excel does. With field headers off there's no Values row, so the writtenfirstDataRowno longer counts one.defaultSubtotal="0"withsumSubtotal="1",countSubtotal="1"…) has subtotals on. On the first row or column field, with noxpdl:pivotTableDefinition16, the same rule sets the table's default subtotal setting. GridCraft doesn't model the functions themselves: it shows the automatic subtotal, with a warning, and saves the field with the automatic subtotal.rowGrandTotals/colGrandTotalswere read and written the wrong way round. A round trip hid this, but files saved by Excel opened with the wrong totals.<d>rather than<n>. One number held in both a date cell and a General cell is a single item. It sorts as a number if any cell in the source column holds it with a non-date format, whatever the row order or report filter. On refresh the flags are read only for ungrouped row and column fields. The field list's item samples (pivot.fields) and the recommended-PivotTable suggestions read them for every source column they list. Either way, each distinct style costs one format lookup. This matches Excel's saved item order for the two mixed fields of ClosedXML'sVariousDataTypesInTableColumns(only_date_formatted_cells_sort_as_dates). Item labels still use the column's first-cell format, so a time in a column whose first cell is General shows as 0.6875 where Excel shows 16:30.pivot.listreports each field'scaption,sortByandnumberFormat.Measured
A throwaway
pivotcmpexample (not committed) opens each of 11 Excel-saved fleet reports, recomputes every PivotTable and compares each cell of the computed report range with the cell Excel saved (numbers within 1e-9 relative, empty equal to empty text). It counts the cells that differ. On this branch it gives 3475 differing cells before ce8f5fc and 3428 after. The whole drop comes from the numeric "For empty cells show" caption: the two pivots that improved are the only ones withmissingCaption="0", and writing that caption as text again (the one-line change incompute) brings back exactly 3475. The sort scope has no measured effect: all 20<autoSortScope>s in the 11 reports reference only the Values field, none an item of a column.missingCaption="0")missingCaption="0")The follow-up fixes (scope keyed by date grouping, vanished scope items, custom subtotals, date order of date groups) leave the total at 3428.
ClosedXML's
VariousDataTypesInTableColumns__input.xlsx(saved by Excel; a time field and a field mixing numbers, text, TRUE, an error, dates and a blank) refreshes with 16 cells different from Excel's saved report, against 55 on main. (Remeasured after the round-2 fix: still 16. Command:/tmp/rv-fix-722/scratch/diffcount.sh <gridcraft-cli> <file>, which runsdata.refreshAlland counts differing cells over A1:AZ200 on every sheet. The remaining differences include the label formats above.)Most of what's left is out of scope for this PR: label/value and Top 10 filters (#498) and the order of items that tie on the sort value (see Not done).
gridcraft-cli checkon the 11 reports, before and after ce8f5fc and after the follow-up fixes: 0 of 11 files differ on round trip, 0 fail to open, and the calc oracle's 2 mismatches are the same.Verification
crates/engine/src/cmd/pivot.rs):sort_by_value_within_each_parent(descending and ascending, nested rows with subtotals, column fields,byvalidation, rename follow),captions_renamed_items_and_manual_order,field_subtotals_repeat_labels_and_values_row,empty_cell_caption_classic_layout_and_date_order,sort_by_value_in_a_column_keeps_its_scope(the column scope sorts the rows, survives an XLSX round trip, and an unsupported two-item scope loads as a sort by label with a warning),design_subtotals_replace_field_settings,numeric_empty_cell_caption_is_a_number,manual_order_of_time_items_from_an_excel_cache(an Excel-style<d v="1899-12-30T08:00:00"/>cache keeps 8:00, 9:30, (blank) through a refresh and a save) anddates_sort_after_logicals_and_errorsandonly_date_formatted_cells_sort_as_dates(Excel's saved item order for ClosedXML's two mixed fields, and the same number in a date cell and a General cell in either row order; fails without the per-cell date flag).crates/xlsx/src/tests/pivot_tests.rs): the round-trip fixtures set every new setting (sort by value with a scope, manual item order, field and item captions, table captions, per-field subtotals overriding the table default, Repeat Item Labels, field number format, Values row, classic layout),pivots_round_tripchecks they read back equal, andpivot_xml_detailschecks the scope's<references>.manual_order_in_cache_order_is_keptandlocation_without_field_headers_has_no_values_rowcover the item order andfirstDataRow, andearly_dates_in_caches_count_real_daysthe dates written before March 1900. Core (crates/core/src/date.rs):iso_dates_count_real_dayscovers the cache date conversion both ways in both date systems.sort_scope_tells_date_levels_of_one_column_apart(a Years + Quarters scope sorts by the Qtr2 column and is written back whole),custom_subtotal_functions_keep_the_subtotal_rows,custom_subtotals_on_the_first_field_keep_the_table_default(the table default stays on and saving adds noEnabledSubtotalsDefault="0"),sort_scope_on_a_vanished_item_sorts_as_after_saving(displayed order equals the order after save and reopen) anddate_groups_not_sorted_are_in_date_order.cargo fmt --all, clippy-D warningson core, model, xlsx and engine (all targets) are clean, and every core, model, xlsx and engine test passes.cargo xtask cipasses all 8 steps (fmt, clippy, test, assets, layers, wasm, parity, keytips) on the head commit ec3666f.Merging with #498
Rebased onto main, which now has #718 (values on rows): the row layout takes both #718's Σ Values row level and this PR's repeat-labels parameter, the "Values" header row is skipped when Σ Values sits in Rows (
!on_rows, in the engine and in the writtenfirstDataRow), and the old "values on rows are shown on columns" warning is gone. #498 (filters) still overlaps inxlsx/pivot.rs,engine/pivot.rsandengine/cmd/pivot.rs: each side adds code next to the other (field_jsongoes with #498'saxis_json, thegetclosure sits next tovalue_at, andfilters_xmlis written before theextLst). Whichever lands second, I'll rebase it.Not done
pivotFielditems, the cache'ssharedItems, the source's first appearance, each reversed or not) explains more than 21 of them, which is no better than the label order. Excel's tie order looks like an artefact of an unstable sort, not a stored order.Review
Six adversarial review rounds ran on this PR.
manual_order_of_time_items_from_an_excel_cache,early_dates_in_caches_count_real_days,iso_dates_count_real_days). (P3) dates sorted between text and logicals; they now sort after errors (b60d37e,dates_sort_after_logicals_and_errors).only_date_formatted_cells_sort_as_dates). The cache writer is unchanged: it writes<d>only for date-grouped fields, so the flag doesn't affect it. The "as in Excel's item lists" claim was corrected.Vec<u32>of record rows instead of aVec<bool>per record. (P3) a date cell and a General cell holding the same number shared one item whose class depended on row order; any General occurrence now makes it a number (round 4 makes that column-wide). Keeping<d>/<n>as separate items was rejected: both would get the same label, and selected/hidden items, captions and manual order are keyed by label. (7fe89ba,only_date_formatted_cells_sort_as_dates.)Source::date_cellsnow decides it over every record of the column before filtering (20a85b2,a_general_cell_filtered_out_still_makes_its_number_sort_as_a_number, which fails on 7fe89ba withABC, 45000). The body now says which callers read the flags for every column, and the RSS note no longer quotes a machine-specific range.date_cellshashed every non-date number into the General set even when no cell of the column was a date, so a numeric General row field paid for it on each refresh. The set is now built only when some flag is set (ec3666f). The timing data below replaces the earlier user-time-only notes.Performance
Every measurement uses release CLIs on the same generated 100k-row × 8-column source: Region and Product text, Date date-formatted, five General number columns. Each run inserts a pivot (rows = the field, Sum of Amount) and refreshes it 0 or 10 times; per refresh = (median at 10 − median at 0) / 10. Builds: 0739f71 (before the per-cell flag), 7fe89ba (round 3), 20a85b2 (round 4), ec3666f (this head).
Instructions retired and cycles (
/usr/bin/time -l) are the primary evidence, because user time moved with machine load. Script:/tmp/rv-fix-722/bench/icount.sh; summary:/tmp/rv-fix-722/bench/stats.py <files>.Instructions per refresh, median of 5 interleaved runs each (
icount-r5.txt). Cycles per refresh in the same runs: Region 213.2M / 221.4M / 223.2M / 212.3M, Date 323.5M / 333.7M / 318.7M / 346.6M, Qty 386.0M / 388.0M / 404.5M / 383.7M. They vary by several percent from run to run, in both directions.The round-5 reviewer measured independently with the same method (
/tmp/rv5-722/icount.txt,icount2.txt,icount3.txt,icount4.txt). Per refresh, 0739f71 / 7fe89ba / 20a85b2:cli-noset): 1499.6M instructions on Qty.User CPU per refresh from the earlier rounds (
/tmp/rv-fix-722/bench/bench.sh), all of it:results.txtresults-r4.txtresults-r4b.txtresults-r4c.txtresults-r4d.txtintern0.118 sThe instruction counts don't show those user-time gaps. For example, Region at 20a85b2 retires 1.4% more instructions than 0739f71, but measured 6-12% more user time. Peak RSS showed no consistent difference between builds.