Skip to content

fix: carry formula cells into rows created by collection fill - #1215

Open
BigDataDZ wants to merge 5 commits into
apache:mainfrom
BigDataDZ:fix/fill-preserve-formula-in-created-rows
Open

BigDataDZ wants to merge 5 commits into
apache:mainfrom
BigDataDZ:fix/fill-preserve-formula-in-created-rows

Conversation

@BigDataDZ

Copy link
Copy Markdown
Contributor

Fixes #199

What changes were proposed in this pull request?

When a template row holds a formula cell next to {data.xxx} placeholders, filling a list created one row per item but wrote only the placeholder columns - the formula silently disappeared from every created row, so formula columns next to the data went empty.

The fill executor now records the formula cells that sit in the collection template rows and recreates each of them in every created row. References are translated with POI's FormulaShifter (row copy for vertical fills, column copy for horizontal fills) the way a spreadsheet copies a row down: relative references follow the new position, absolute ones stay anchored. Cell style is carried over under the existing autoStyle behavior, and a formula that cannot be parsed or translated is skipped instead of failing the fill.

How was this patch tested?

  • New FillWithFormulaInCreatedRowsTest: a programmatic template mixing a relative formula (A2*2) and an absolute one ($A$2*10) with {data.name}; filling 3 items with and without forceNewRow now asserts the created rows carry A3*2 / A4*2 while $A$2*10 stays anchored.
  • Full module suite green: 1106 tests, 0 failures (mvn install -Dmaven.test.skip=false, JDK 25).

Generated-by: ZCode

Signed-off-by: BigDataDZ 76271875+BigDataDZ@users.noreply.github.com

A template row that mixes {data.xxx} placeholders with a formula cell
lost the formula in every row created by a list fill: template analysis
only inspects string cells, and the fill only writes the placeholder
columns into the created rows.

The fill executor now records the formula cells of the collection
template rows and recreates them in each created row, translating the
formula with POI's FormulaShifter (row/column copy) so relative
references follow the new position and absolute ones stay anchored.
Styles follow the existing autoStyle behavior and unparsable formulas
are skipped instead of failing the fill.

Fixes apache#199

Generated-by: ZCode
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
The CI check job failed spotless:check on the test file and the fill
executor. Re-ran spotless:apply; no logic changes.

Generated-by: ZCode

@skytin1004 skytin1004 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigDataDZ Thanks for working on this. While testing it locally, I found that formulas can still be missed when there are multiple collection areas on the same sheet. I left an inline comment with the details. Could you take a look?

}
}
}
collectFormulaTemplateCells(sheet);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial template scan populates the analysis cache for all collection keys, but this only collects formula cells for the current collection. A later collection can therefore hit the early return above before this method runs for that collection.

I reproduced this locally with two collection areas on the same sheet: formulas were copied for the first collection, but the formula next to the second collection was missing.

Could we make sure formula cells are collected for each collection even when its analysis data is already cached, and add a regression test covering multiple collection areas?

The full template scan fills the analysis cache for all collection keys
at once, so every later collection hits the early return in
readTemplateData and skipped formula collection: only the first filled
collection kept its formulas. Collect the formula cells on the cached
path too, guarded so the same collection never collects twice.

Adds a regression test with two collection areas on one sheet: the
collection filled second (its analysis data already cached) now carries
its formulas into created rows as well. The test uses the writer-level
fill API because ExcelWriterSheetBuilder.doFill finishes the writer on
every call.

Review: skytin1004 on apache#1215.

Generated-by: ZCode
@BigDataDZ

Copy link
Copy Markdown
Contributor Author

@skytin1004 Thanks for catching this — you were right on both points. Fixed in 906bc04:

  1. Collection on the cached path: readTemplateData now calls collectFormulaTemplateCells on the early-return path too, so a collection whose analysis data is already cached still collects its formula cells before its rows are filled. The collector got an idempotency guard (templateFormulaCellsCache.containsKey) so a collection can never append the same formula twice.

  2. Regression test: fillKeepsFormulasForEveryCollectionArea covers two collection areas on one sheet with the second one filled after the first (its analysis data already cached), asserting both areas keep formulas in created rows. It fails with expected: <A3*2> but was: <null> on the previous code and passes with the fix. One note for the test: it drives the writer-level fill() API twice instead of ExcelWriterSheetBuilder.doFill(), since the builder finishes the writer after every call — with the builder, the second collection was silently dropped entirely (values included), which hid the bug behind a different symptom.

Full fesod-sheet suite passes (1047 tests, 0 failures); spotless clean.

@skytin1004 skytin1004 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick follow-up @BigDataDZ. I verified that 906bc04 fixes the cached-analysis case I originally reported, and the new multi-collection regression test correctly covers that failure.

While re-reviewing the full formula-copy path, I found two separate correctness issues that I missed in my first pass. Sorry for the extra round, but both can result in incorrect formulas or lost data, so I'm requesting changes once more. Please take a look at the inline comments.

}
}
}
collectFormulaTemplateCells(sheet);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reproduced a forceNewRow case where a formula references a row below the collection area. Formula cells are cached here before shiftRows() runs, so TemplateFormulaCell retains the pre-shift formula text even though POI updates the formula in the workbook.

For example, with B2 = A4*2, filling two items with forceNewRow(true) correctly updates the original B2 to A5*2, but the copied B3 is also A5*2 instead of the expected A6*2.

Could we invalidate or refresh the formula cache after structural row shifts and add a regression test for this case?

vertical ? templateFormulaCell.getRowIndex() + relativeRowIndex : templateFormulaCell.getRowIndex();
int targetColumnIndex = vertical
? templateFormulaCell.getColumnIndex()
: templateFormulaCell.getColumnIndex() + relativeRowIndex;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also reproduced a collision in horizontal fill. With {data.name} in A2, =A2*2 in B2, and three horizontally filled items, B2 ends up as a FORMULA cell rather than the second item's STRING cell. The value only survives as the formula's cached result and is lost when the workbook recalculates.

Formula discovery is currently row-based while horizontal collections expand by column. Could we make discovery and placement direction-aware and add a horizontal regression test that verifies both cell types/values and formulas after reopening the workbook?

Two correctness issues found in re-review of the formula-copy path:

1. Horizontal fill collision: collectFormulaTemplateCells scanned the
   full collection row, so formulas in data columns were collected and
   later overwrote filled data. Columns holding collection placeholders
   are now excluded from formula discovery.

2. forceNewRow formula staleness: shiftRows may update formula
   references in the live sheet (e.g. =A4*2 becomes =A5*2 when the
   referenced row shifts), but TemplateFormulaCell cached the pre-shift
   text before shiftRows ran. The cache is now refreshed after structural
   row shifts by adjusting row references in the cached formula text.

Adds regression tests for both: horizontal fill preserves data cells;
forceNewRow translates the post-shift formula into created rows.

Review: skytin1004 on apache#1215 (second round).

Generated-by: ZCode
@BigDataDZ

Copy link
Copy Markdown
Contributor Author

@skytin1004 Both issues confirmed and fixed in 4902242:

  1. Horizontal fill collision: collectFormulaTemplateCells now skips columns that hold collection placeholders, so the data column is never overwritten by a formula copy. Regression test horizontalFillDoesNotCollideWithDataColumn verifies data cells survive after reopening.

  2. forceNewRow formula staleness: after shiftRows the cached formula text is refreshed by shifting row references (the regex handles both A4 and $A$4 style refs). Regression test forceNewRowUpdatesFormulaCacheAfterShift verifies the created row translates the post-shift formula (A6*2) rather than the stale pre-shift text (A5*2).

Full fesod-sheet suite: 1049 tests, 0 failures. Spotless clean.

Thanks for the thorough second pass — both were real bugs worth catching.

Matcher.appendReplacement accepts only StringBuffer on JDK 8; the
StringBuilder variant is JDK 9+. The project compiles on JDK 8 in CI.

Generated-by: ZCode

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Fill behavior is not preserved for formula cells in a row

2 participants