Add AreaReference support for structured table references - #1750
ken-swyfft wants to merge 10 commits into
Conversation
…erence() AreaReference and CellReference only handle A1-style cell references. When a named range's RefersToFormula contains a structured table reference (e.g., Table7[#Headers], Table1[[#Data],[Column1]]), the AreaReference constructor throws ArgumentException. This adds two static methods to AreaReference: - IsStructuredReference(string) - detects structured reference syntax - CreateFromStructuredReference(string, version, workbook, rowIndex) - resolves structured refs to concrete AreaReference by delegating to the existing FormulaParser.ParseStructuredReference() The existing constructor behavior is unchanged - callers must opt in to structured reference support by using the new methods. Includes 12 tests covering #Headers, #Data, #All, #This Row, single column, column range, headers+column, and error cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DateTime.UtcNow has sub-second precision. ToString("HH:mm:ss") truncates
(e.g., 16:56:17.987 -> "16:56:17"), but DataFormatter rounds the Excel
OLE date double, producing "16:56:18". Truncate to whole seconds before
the test starts so both paths agree.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace CreateFromStructuredReference() with a constructor overload: new AreaReference(reference, version, workbook, rowIndex) The existing 2-param constructor chains to the new 4-param one. When a workbook is provided and the reference is a structured table reference, it resolves automatically. Regular references pass through unchanged. Without a workbook, behavior is identical to before. This is more ergonomic for callers — structured references "just work" without needing to check IsStructuredReference() first. Tests expanded from 12 to 18: added coverage for constructor chaining with regular refs (single cell, multi-cell, sheet-qualified, absolute, whole-column), null workbook pass-through, and workbook + regular ref. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@tonyqus - Just as an FYI, we ran into this in a production scenario that wasn't adequately covered by our tests, so it hadn't turned up in our previous testing. |
ken-swyfft
left a comment
There was a problem hiding this comment.
Overall: Approve. Clean, well-scoped change that solves a real problem. Reuses existing FormulaParser infrastructure rather than duplicating structured reference parsing. Constructor chaining preserves full backward compatibility. Test coverage is thorough — especially the backward compat tests after the constructor refactor.
A few optional suggestions inline — none are blocking.
- Document possible exceptions (KeyNotFoundException, FormulaParseException, InvalidOperationException) in constructor XML doc - Add performance note about FormulaParser allocation in remarks - Clarify isAbsolute/isRelative inversion with inline comment - Note regex false-positive behavior in code comment - Document exception types in test for invalid table name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds structured table reference support to AreaReference by introducing a workbook-aware constructor overload that resolves Excel table references (e.g., Table1[#Headers]) into concrete cell ranges, plus a helper to detect structured references. Also stabilizes a flaky timestamp-related test.
Changes:
- Add
AreaReference(String, SpreadsheetVersion, IFormulaParsingWorkbook, int rowIndex = 0)to resolve structured references viaFormulaParser.ParseStructuredReferencewhen a workbook is provided. - Add
AreaReference.IsStructuredReference(string)for structured reference detection. - Fix
TestNPOI1469flakiness by truncating timestamps to whole seconds before formatting/comparison.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
main/SS/Util/AreaReference.cs |
Adds workbook-aware parsing path for structured references and exposes structured-reference detection helper. |
testcases/ooxml/SS/Util/TestAreaReferenceStructuredReferences.cs |
New NUnit tests covering structured resolution, backward compatibility, and workbook + regular references. |
testcases/main/HSSF/UserModel/TestCellStyle.cs |
Stabilizes TestNPOI1469 by avoiding sub-second rounding mismatches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uredReference Fully qualify the `KeyNotFoundException` cref so it resolves without needing a `using System.Collections.Generic;` in this file. Make `IsStructuredReference` return false for null input and require the regex to span the entire string, so an embedded bracketed substring in a larger value does not false-positive into the structured-reference parse path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…a-reference-structured-table-refs
Four changes from an independent review of nissl-lab#1750: - rowIndex now defaults to -1 ("not specified") instead of 0. With 0, a [#This Row] reference silently resolved against the header row of a table starting at row 0, and threw the opaque "Illegal structured reference" for any table that did not. -1 produces FormulaParser's explicit "Row index must be specified" error instead. - Document and test the InvalidOperationException cases. [#Totals] on a table with no totals row, and [#This Row] with a rowIndex outside the table, are #REF!/#VALUE! during evaluation; a constructor has no error-value channel, so they surface as exceptions. The XML doc now names both causes rather than only the generic condition. - Add AreaReference.FromStructuredReference as an explicit counterpart to the constructor. It derives the spreadsheet version from the workbook (the constructor's version argument is ignored on the structured path, now documented) and rejects non-structured input instead of silently falling back to A1 parsing. The transparent constructor is unchanged, so callers resolving a named range of unknown form still need no branching. - BaseXSSFEvaluationWorkbook.GetTable now returns null for an unknown table name, as its doc comment already promised, instead of letting the dictionary indexer throw KeyNotFoundException. This makes FormulaParser's existing null check reachable, so a bad name reports itself as FormulaParseException naming the table. It also fixes INDIRECT: Indirect only catches FormulaParseException, so KeyNotFoundException previously escaped it and failed evaluation instead of yielding #REF!. Covered by a new test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Indirect caught only FormulaParseException around ParseStructuredReference, so the InvalidOperationException raised for a structured reference that parses to an error Ptg escaped the handler and failed evaluation outright. Two ordinary workbook shapes hit this: [#Totals] on a table with no totals row, and [#This Row] or @ on a row outside the table. Both now return ErrorEval.REF_INVALID, matching how the adjacent FormulaParseException case is already reported and how Excel surfaces an INDIRECT reference it cannot resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tructured-table-refs
…tructured-table-refs
Summary
Adds a constructor overload to
AreaReferencethat transparently handles Excel structured table references (e.g.,Table7[#Headers],Table1[[#Data],[Column1]]):When a workbook is provided, structured table references are detected and resolved automatically against the workbook's table definitions. Regular A1-style references pass through unchanged. Without a workbook, behavior is identical to before.
Also adds
AreaReference.IsStructuredReference(string)for callers that need to detect structured references without resolving them.Includes a fix for flaky
TestNPOI1469(timestamp rounding, unrelated).Problem
When a named range's
RefersToFormulacontains a structured table reference,new AreaReference(formula, version)throwsArgumentExceptionbecauseCellReference.SeparateRefPartsonly handles A1-style references. NPOI already has full structured reference parsing inFormulaParser, but it was only accessible during formula evaluation — not for generic reference resolution fromAreaReference.This is a real-world issue: Excel workbooks that use Tables (ListObjects) often have named ranges pointing to structured references like
Table7[#Headers]orTable7[[#Headers],[Column1]]. Code that resolves named ranges to cell positions viaAreaReferencecrashes on these workbooks.Usage
No special branching needed — structured references "just work."
For [#This Row] references that depend on the formula's row position, pass the row index:
Approach
The existing 2-parameter constructor chains to a new 4-parameter overload:
When
workbookis non-null and the reference matches structured reference syntax, the constructor delegates to the existingFormulaParser.ParseStructuredReference(). Otherwise, the original A1-style parsing runs unchanged.Tests
18 tests in
TestAreaReferenceStructuredReferencesorganized in three groups:Structured reference resolution (with workbook):
#Headers,#Data,#All,#This RowspecifiersKeyNotFoundExceptionBackward compatibility (without workbook):
ArgumentExceptionWorkbook + regular references:
All existing tests continue to pass.