fix(xls/xlsb): correct column letters in reconstructed formulas (columns >= AA) - #699
Merged
jmcnamara merged 3 commits intoAug 14, 2026
Merged
Conversation
gaoflow
force-pushed
the
fix-xls-formula-column-refs-beyond-z
branch
from
July 29, 2026 02:58
7d2e04f to
cc036b5
Compare
jmcnamara
requested changes
Jul 29, 2026
Collaborator
|
As a general comment for future submissions, fixes for an issue should start with a bug report. This change also contains two separate bugs/PRs. |
jmcnamara
added
the
needs work for merge
push_column dropped the most-significant digit for columns after Z (e.g. index 26 rendered as "B"). Share one digit-fill implementation with the xlsx converter, without allocating a buffer per call.
PtgArea column fields pack 14-bit column numbers with fColRel/fRwRel flags; the old code read the raw 16 bits, so columns >= 64 (and the relative flags) were wrong.
gaoflow
force-pushed
the
fix-xls-formula-column-refs-beyond-z
branch
from
July 31, 2026 18:13
cc036b5 to
060bed4
Compare
Contributor
Author
|
All applied: push_column and column_number_to_name now share one allocation-free digit-fill core, comments trimmed, test renamed to test_push_column (BA case and typos entry dropped), and the branch is split into two commits — fix(utils) and fix(xls). Kept them in one PR since the xls fix builds on the column rendering; happy to split into a separate PR if you prefer. |
Collaborator
|
Thank you. Could you also mark the comments as resolved. |
Contributor
Author
|
All review threads are now marked resolved. Thanks! |
Collaborator
|
Merged. Thanks. |
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.
Problem
utils::push_columnconverts a 0-based column index to Excel column letters, but it used plain base-26 and dropped the most-significant digit, so every index >= 26 was wrong:AAAABAAZZZBAAAA775 of the first 801 indices disagree with Excel; only
A..Z(0..25) were correct.push_columnis the renderer behind all ~13 column references emitted by the.xlsand.xlsbformula readers, soworksheet_formulareturned silently wrong columns for any binary-format formula referencing columnAAor beyond, e.g.=SUM(AA1:AA10)came back as=SUM(A1:A10)— a different column, no error. (xlsx/ODS are unaffected; they carry formulas as strings.)Excel columns are bijective base-26, and the crate already implements this correctly in
xlsx::column_number_to_name(with a passing test).push_columnjust never matched its sibling. The fix mirrors that function; a new test asserts the two are byte-identical across the whole valid column range.Second bug in the same path
While adding an integration fixture I found the BIFF8
PtgAreabranch passed the raw column word — including thefColRel/fRwRelflag bits — straight topush_columnand hardcoded$, soSUM(AA1:AA3)rendered as$BTSM$1:$BTSM$3. It now masks the 14-bit column and reads the relative flags, mirroring the adjacentPtgRefbranch that already does this.The 3D-reference branches (
PtgRef3d/PtgArea3d) have the same unmasked-column pattern and a pre-existing// TODO: check with relative columns; I left those for a separate change to keep this focused.Tests
push_column_is_bijective_base26— boundary cases (0->A,25->Z,26->AA,701->ZZ,702->AAA,16383->XFD) plus a differential check againstxlsx::column_number_to_nameover the full column range.xls_formula_columns_beyond_z— a new.xlsfixture whose formulas reference columns >= 26 (existing formula fixtures only use single-letter columns, so this path was untested). RendersSUM(AA1:AA3)andAA1+AB1.Full suite passes.
This PR was written with AI assistance under my direction; I have reviewed it and am accountable for it.