Skip to content

fix: treat numRows zero as unlimited - #1159

Merged
bengbengbalabalabeng merged 4 commits into
apache:mainfrom
Aias00:fix/numrows-zero-unlimited-1149
Nov 6, 2026
Merged

bengbengbalabalabeng merged 4 commits into
apache:mainfrom
Aias00:fix/numrows-zero-unlimited-1149

Conversation

@Aias00

@Aias00 Aias00 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closed: #1149

Summary

  • make numRows(0) follow the documented unlimited behavior
  • cover workbook-level and sheet-level numRows(0) reads across formats

Verification

  • ./mvnw -pl fesod-sheet -Dmaven.test.skip=false -DskipTests=false -Dtest=ParameterDataTest#numRowsZeroReadsAllRows test
  • ./mvnw -pl fesod-sheet spotless:check

Signed-off-by: liuhy <liuhongyu@apache.org>
@BigDataDZ

Copy link
Copy Markdown
Contributor

Small suggestion from a fellow traveler on #1149 (my overlapping #1189 — closing it in favor of this one, the core guard is identical). While testing I also covered two neighboring cases that might be worth guarding here: numRows omitted entirely (must keep reading all rows) and a positive limit (must still stop early), plus a headless-sheet setup so the limit maps 1:1 onto data rows. Happy to send these as a follow-up PR if useful.

@BigDataDZ BigDataDZ 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.

The core guard is exactly right - limit > 0 matches the documented sentinel, and it is the same shape I had shipped in the closed #1189 (verified there against 9 format/level combinations).

Two of the neighboring cases from the Oct-9 comment are still uncovered, and they protect the other half of the condition:

  1. A positive limit must still stop the read - limit > 0 exists to disable the zero sentinel, but nothing pins that a legitimate numRows(1) still truncates. Without that case, a future "simplification" to limit == null || limit <= 0 || index < limit-style refactors could silently read everything.
  2. Omitted numRows stays unlimited - the limit == null fall-through from sheet holder to workbook holder.

One testing detail from #1189 that will save you an iteration on case 1: lastRowIndex counts the head row (a headed file's first data row sits at index 1), so numRows(1) on a headed default read delivers zero data rows. Writing a header-less file (or headRowNumber(0)) makes the limit map one-to-one onto data rows - my closed branch has ready-made parameterized tests for all three cases (zero / omitted / positive across XLSX, XLS, CSV) if you want to lift them: BigDataDZ/fesod@f4e0ec7 (NumRowsReadLimitTest).

Happy to review the follow-up commit. Otherwise LGTM on the fix itself.

@bengbengbalabalabeng bengbengbalabalabeng 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.

LGTM.

@bengbengbalabalabeng
bengbengbalabalabeng merged commit ea04386 into apache:main Nov 6, 2026
9 checks passed
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] numRows(0) stops reading instead of disabling the row limit

3 participants