Skip to content

fix: fix error in using relativeHeadRowIndex for writing tables only#810

Open
32154678925 wants to merge 11 commits into
apache:mainfrom
32154678925:fix/fix-relativeHeadRowIndex-invalid-#794
Open

fix: fix error in using relativeHeadRowIndex for writing tables only#810
32154678925 wants to merge 11 commits into
apache:mainfrom
32154678925:fix/fix-relativeHeadRowIndex-invalid-#794

Conversation

@32154678925

@32154678925 32154678925 commented Jan 18, 2026

Copy link
Copy Markdown

Purpose of the pull request

Closed: #794
When you do not write directly in the sheet, but only write data in the table and set the head, the offset value set with relativeHeadRowIndex() will make an error, and the blank line with the specified offset will be empty again between the head and the data. If the head is not set, the offset value set with relativeHeadRowIndex() will not take effect

What's changed?

Change the judgment of whether to add offset in the add method in the ExcelWriteAddExecutor class from writeSheetHolder to currentWriteHolder

Before:

WriteSheetHolder writeSheetHolder = writeContext.writeSheetHolder();
int newRowIndex = writeSheetHolder.getNewRowIndexAndStartDoWrite();
if (writeSheetHolder.isNew()
        && !writeSheetHolder.getExcelWriteHeadProperty().hasHead()) {
    newRowIndex += writeContext.currentWriteHolder().relativeHeadRowIndex();
}

After:

WriteSheetHolder writeSheetHolder = writeContext.writeSheetHolder();
WriteHolder currentWriteHolder = writeContext.currentWriteHolder();
int newRowIndex = writeSheetHolder.getNewRowIndexAndStartDoWrite();
if (currentWriteHolder.isNew()) {
    AbstractWriteHolder writeHolder = (AbstractWriteHolder) currentWriteHolder;
    if(!writeHolder.getExcelWriteHeadProperty().hasHead()){
        newRowIndex += currentWriteHolder.relativeHeadRowIndex();
    }
}

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Copilot AI 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.

Pull request overview

This pull request fixes a bug where relativeHeadRowIndex() was being applied twice when writing data to tables with headers, causing extra blank rows to appear between the header and data. The fix changes the condition from checking whether the sheet is new to checking whether the current write holder (which could be a table) is new and has no head.

Changes:

  • Modified logic in ExcelWriteAddExecutor.add() to check currentWriteHolder instead of writeSheetHolder for applying the row index offset
  • Added import for AbstractWriteHolder class
  • Added instanceof check before casting to AbstractWriteHolder (unnecessary based on codebase architecture)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +70 to +74
if (currentWriteHolder instanceof AbstractWriteHolder) {
AbstractWriteHolder writeHolder = (AbstractWriteHolder) currentWriteHolder;
if (!writeHolder.getExcelWriteHeadProperty().hasHead()) {
newRowIndex += currentWriteHolder.relativeHeadRowIndex();
}

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

The instanceof check for AbstractWriteHolder is unnecessary. All implementations of WriteHolder (WriteSheetHolder, WriteTableHolder, WriteWorkbookHolder) extend AbstractWriteHolder. The rest of the codebase directly casts currentWriteHolder() to AbstractWriteHolder without instanceof checks (see WriteHandlerUtils.java lines 79, 124, 160, 168, 176, 184). For consistency with the existing codebase patterns, the cast should be done directly without the instanceof check.

Suggested change
if (currentWriteHolder instanceof AbstractWriteHolder) {
AbstractWriteHolder writeHolder = (AbstractWriteHolder) currentWriteHolder;
if (!writeHolder.getExcelWriteHeadProperty().hasHead()) {
newRowIndex += currentWriteHolder.relativeHeadRowIndex();
}
AbstractWriteHolder writeHolder = (AbstractWriteHolder) currentWriteHolder;
if (!writeHolder.getExcelWriteHeadProperty().hasHead()) {
newRowIndex += currentWriteHolder.relativeHeadRowIndex();

Copilot uses AI. Check for mistakes.
@bengbengbalabalabeng

Copy link
Copy Markdown
Contributor

These tests contain no assertions and therefore do not validate the fix. Please enhance them with assertions that check the actual output. (Also ensure that both XLSX and XLS formats are covered)

 - Add read-back verification via WorkbookFactory for both test cases
 - Parameterize with @EnumSource to cover both .xls and .xlsx formats
 - Use randomized offsets (0–9) and data sizes (1–10) for broader coverage
 - Replace non-deterministic Random data with predictable prefix‑index pattern
 - Resolve header string from @ExcelProperty annotation via reflection
   to avoid coupling to a hardcoded value in WriteSheetData
 - Calculate expected sequential row layout (tables are sequential, not
   overlapping; B's position = A's last row + offsetB + 1)

Signed-off-by: 苗泽楠 <anan_laolu@foxmail.com>
Signed-off-by: 苗泽楠 <anan_laolu@foxmail.com>
@32154678925

Copy link
Copy Markdown
Author

这些测试不包含断言,因此不验证修复。请用检查实际输出的断言来增强它们。(同时确保涵盖XLSX和XLS格式)

谢谢,我修改了测试代码,并且成功跑通测试,且测试了未修改bug的代码,测试是跑不同的。我的代码经验不多,还有什么需要修改的么?

Thank you. I modified the test code and successfully ran through the test. I also tested the code without modifying the bug. The test runs differently. I don't have much code experience. Is there anything I need to change?

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] 使用table的relativeHeadRowIndex()设置写入行偏移量时效果不对

4 participants