Skip to content

fix: correct CSV physical cell count#907

Merged
delei merged 2 commits intoapache:mainfrom
skytin1004:test/csv-row-physical-cell-count
May 5, 2026
Merged

fix: correct CSV physical cell count#907
delei merged 2 commits intoapache:mainfrom
skytin1004:test/csv-row-physical-cell-count

Conversation

@skytin1004
Copy link
Copy Markdown
Contributor

@skytin1004 skytin1004 commented Apr 30, 2026

Purpose of the pull request

While looking at the CSV row API from a workbook write handler, I noticed that CsvRow#getPhysicalNumberOfCells() was returning the row index instead of the number of cells in the row.

I checked the implementation and found that getPhysicalNumberOfCells() was calling getRowNum(). That means the first row returns 0, even when the row actually contains cells. For example, a CSV header row with No, Name, and Age should report 3 physical cells, but it reported 0.

What's changed?

I changed CsvRow#getPhysicalNumberOfCells() to return cellList.size() instead of getRowNum().

I also added a regression test in CsvFormatTest. The test writes a CSV file with three header cells, then checks the first row from a workbook write handler and verifies that getPhysicalNumberOfCells() returns 3.

Tested with CsvFormatTest#testPhysicalNumberOfCells, full CsvFormatTest, and spotless:check.

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.

@alaahong alaahong requested a review from Copilot April 30, 2026 14:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


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

Copy link
Copy Markdown
Member

@delei delei left a comment

Choose a reason for hiding this comment

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

LGTM

@delei delei merged commit f673fcb into apache:main May 5, 2026
13 checks passed
@skytin1004 skytin1004 deleted the test/csv-row-physical-cell-count branch May 5, 2026 21:26
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.

3 participants