feat: add support for column index limit in ReadSheet by sapienza88 · Pull Request #942 · apache/fesod

sapienza88 · 2026-07-05T08:41:42Z

Purpose of the pull request

Related: #950

What's changed?

Added support for column-based reading.
Currently xlsx only; xls support will be added later.

Column filtering behavior: CellTagHandler#endElement

When includeColumnIndexes != null:

Only process target columns
Rewrite target columns’ internal indexes to consecutive (0-based)

When includeColumnIndexes == null:

Keep the original behavior

API Usage Example

// data
┌────────────┬─────────────┬────────────┬─────────────┐
│     NO     │     Name    │     Age    │   Password  │
├────────────┼─────────────┼────────────┼─────────────┤
│     1      │    Jackson  │     22     │    123456   │
└────────────┴─────────────┴────────────┴─────────────┘

List<Map<Integer, String>> results = FesodSheet.read(pathname)
		.sheet()
		.includeColumnIndexes(Arrays.asList(1, 3))
		.doReadSync();

Map<Integer, String> data = results.get(0);

assertEquals(2, data.size());
assertEquals("Jackson", data.get(0));
assertEquals("123456", data.get(1));

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.

delei · 2026-07-05T09:51:33Z

Welcome to participate in open source contributions.

Please refer to the contribution guide, complete your PR content and modify the commit message format, and provide the unit test code.

…es: add test

bengbengbalabalabeng · 2026-07-05T11:59:18Z

Please refer to the code contribution. Make sure the changes can be compiled and are properly formatted :)

sapienza88 · 2026-07-05T12:03:34Z

Please refer to the code contribution. Make sure the changes can be compiled and are properly formatted :)

No I cannot make sure it compiles on CI, maybe locally but not on CI

bengbengbalabalabeng · 2026-07-05T12:11:52Z

No I cannot make sure it compiles on CI, maybe locally but not on CI

To prevent CI from being blocked by formatting or compilation issues, you can first run mvn clean install -DskipTests and mvn spotless:apply locally to ensure that both the build and formatting pass.

Please refer to the code contribution.

delei · 2026-07-05T12:26:28Z

No I cannot make sure it compiles on CI, maybe locally but not on CI

Hi, may I ask if this is a draft PR?

I haven't seen any complete implementation logic for this new parameter, and there are no use cases of reading Excel files in the unit test code either.

sapienza88 · 2026-07-05T12:40:33Z

No I cannot make sure it compiles on CI, maybe locally but not on CI

Hi, may I ask if this is a draft PR?

I haven't seen any complete implementation logic for this new parameter, and there are no use cases of reading Excel files in the unit test code either.

not a draft PR, i will extend it to add a complete implementation logic for the new parameter, as well as a test based on reading dummy data.

But also it depends what do you mean by "complete implementation logic"?

delei · 2026-07-05T12:59:30Z

not a draft PR, i will extend it to add a complete implementation logic for the new parameter, as well as a test based on reading dummy data.

It's okay. Please continue to develop and refine this PR, and conduct unit tests in the local environment. When a PR is ready for code review by community members, you can leave a message to let us know.

sapienza88 · 2026-07-08T16:09:50Z

@delei @bengbengbalabalabeng my understanding is that fesod does not support reading a sheet by column (only by rows) but it can write to a square set of cells, is that correct? where this logic of the this PR should also be propagated? which classes are also concerned with reading with a column index?
PS: I added some tests here.

delei · 2026-07-09T05:53:01Z

my understanding is that fesod does not support reading a sheet by column (only by rows) but it can write to a square set of cells, is that correct? where this logic of the this PR should also be propagated? which classes are also concerned with reading with a column index? PS: I added some tests here.

Reading and converting the data in cells is achieved through the ReadListener interface. Please refer to this method to see if it provides any inspiration or assistance to you.

fesod/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/listener/ModelBuildEventListener.java

Line 65 in d7fae5a

private Object buildNoModel(

BTW, I'm a little worried about this PR. Could you please start the preliminary discussion in the issue and provide your proposed solution? (e.g. #941 )

I didn't mean any offense, the other members of the community generally do not discuss proposals in the PR. This would help save the time for the code review.

sapienza88 · 2026-07-09T06:30:55Z

my understanding is that fesod does not support reading a sheet by column (only by rows) but it can write to a square set of cells, is that correct? where this logic of the this PR should also be propagated? which classes are also concerned with reading with a column index? PS: I added some tests here.

Reading and converting the data in cells is achieved through the ReadListener interface. Please refer to this method to see if it provides any inspiration or assistance to you.

fesod/fesod-sheet/src/main/java/org/apache/fesod/sheet/read/listener/ModelBuildEventListener.java

Line 65 in d7fae5a

private Object buildNoModel(

BTW, I'm a little worried about this PR. Could you please start the preliminary discussion in the issue and provide your proposed solution? (e.g. #941 )

I didn't mean any offense, the other members of the community generally do not discuss proposals in the PR. This would help save the time for the code review.

@delei I meant not "in n cells" but in rows AND columns. And not a listnener function. That was a severe limitation of fesod thankfully this PR comes to the rescue, the idea is clear and simple (read a sheet by rows and columns) i think there is no need to describe it more

sapienza88 · 2026-07-09T10:50:05Z

@delei can you review the pr?

delei · 2026-07-09T12:14:15Z

can you review the pr?

Please fix the errors identified in the CI and ensure that the CI can pass successfully.

delei · 2026-07-09T12:26:18Z

+        Assertions.assertNotNull(readResults);
+        Map<Integer, String> parsedRow = readResults.get(0);
+        Assertions.assertEquals("1", parsedRow.get(0));
+        Assertions.assertEquals("Alice", parsedRow.get(1));


List<Integer> targetColumns = Arrays.asList(0, 2);

The targetColumns variable has been set to only include columns 0 and 2, why are there three columns of data in the result?

Is there any misunderstanding or unfinished code logic?

I will correct, i wrongly thought that it is from to

I would like to know what features you have implemented through this PR.

Is it the case that the readResults variable only contains data from these two columns?

Assertions.assertEquals(2,parsedRow.size());

Yes thats correct

@delei fixed now

@delei fixed!

Can we continue to develop it so that it can support xls files at the same time?

@delei I will do that in a separate PR, pls merge this xlsx work first

Thank you for your willingness to continue contributing to open source.

IMO, due to the introduction of new parameters, I hope this is a complete PR that includes all the features, making it easier for community members to understand this parameter.

You can also continue to submit new PR to add documentation for this parameter, optimize the code, or fix minor errors.

@delei sure I will add docs as well for this (in a separate PR), thanks

…into column_support

sapienza88 · 2026-07-14T10:19:34Z

@delei are we done here? can you merge this (with the associated docs: #949)

delei

LGTM.

BTW, currently, only xlsx files are supported.

sapienza88 · 2026-07-15T05:06:48Z

LGTM.

BTW, currently, only xlsx files are supported.

@delei yes

sapienza88 · 2026-07-16T16:15:11Z

@bengbengbalabalabeng all your reviews have been addressed by now

bengbengbalabalabeng · 2026-07-17T11:47:24Z

-
        tempCellData.checkEmpty();
        tempCellData.setRowIndex(xlsxReadSheetHolder.getRowIndex());
-        tempCellData.setColumnIndex(xlsxReadSheetHolder.getColumnIndex());


Considering the earlier logic, assigning tempCellData.setColumnIndex(targetColumnIndex) here might make the behavior more consistent. What do you think :)

targetColumnIndex is assigned in an if clause, so it would not make sense to assign here for all cases.

Considering the earlier logic, assigning tempCellData.setColumnIndex(targetColumnIndex) here might make the behavior more consistent.

Sorry for the confusion, my previous comment might not have been clear enough. Here is what I actually meant:

// ... if (includeColumnIndexes == null) { targetColumnIndex = xlsxReadSheetHolder.getColumnIndex(); } else { // If it's a target column, rewrite the cell's internal index and pack it sequentially targetColumnIndex = includeColumnIndexes.indexOf(xlsxReadSheetHolder.getColumnIndex()); if (targetColumnIndex < 0) { return; } } // ... // line: 136 (old 154) tempCellData.setColumnIndex(targetColumnIndex);

Since we guarantee targetColumnIndex is resolved in both branches, placing setColumnIndex() outside the block maintains the exact same logic while avoiding code duplication. What do you think :)

Yes, that is correct and more readable.

@bengbengbalabalabeng the refactoring has been implemented now, you can merge this PR as well as this related passing test: #945

In the previous version, when includeColumnIndexes != null, the assignment of tempCellData.setColumnIndex() was the overridden targetColumnIndex. Now, however, tempCellData.setColumnIndex() always uses the original xlsxReadSheetHolder.getColumnIndex() instead.

I want to confirm whether this change is intentional. Would you mind clarifying the expected behavior here?

I must have overlooked this, now it is changed as it was in the previous correct version.

bengbengbalabalabeng · 2026-07-19T12:05:28Z

Hi @sapienza88,

Please help confirm whether the updated PR description accurately reflects the changes.

sapienza88 · 2026-07-19T12:12:07Z

Hi @sapienza88,

Please help confirm whether the updated PR description accurately reflects the changes.

@bengbengbalabalabeng it absolutely does, thank you for adding it

bengbengbalabalabeng

LGTM.

Merge in 24h.

read a sheet accounting for a column index limit

cfdb817

delei added the PR: first-time contributor first-time contributor label Jul 5, 2026

feat(sheet): add support for reading sheet with specific column index…

f907ae1

…es: add test

feat(sheet): spotless:apply and CI fixes

12ac741

delei added the PR: developing This feature will be added in future releases label Jul 5, 2026

Selim Soufargi added 2 commits July 5, 2026 15:05

feat(sheet): spotless:apply and add test with data

6e5f7ec

feat(sheet): add import

2f71ecf

Merge branch 'main' into column_support

a466b1e

delei reviewed Jul 9, 2026

View reviewed changes

Selim Soufargi added 7 commits July 9, 2026 21:14

fix CI

3b532dd

Merge branch 'column_support' of https://github.com/sapienza88/fesod …

2480a2e

…into column_support

fix tests

da29379

fix tests

3af1b47

fix tests

bee9c90

fix tests

1c5da7b

cleanup + spotless

a70c56c

delei changed the title ~~Add support for column index limit in ReadSheet~~ feat: add support for column index limit in ReadSheet Jul 12, 2026

delei mentioned this pull request Jul 15, 2026

[Task] add support for column index limit in ReadSheet #950

Open

4 tasks

delei previously approved these changes Jul 15, 2026

View reviewed changes

delei removed the PR: developing This feature will be added in future releases label Jul 15, 2026

bengbengbalabalabeng reviewed Jul 15, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java Outdated

bengbengbalabalabeng reviewed Jul 15, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/ReadSheet.java

bengbengbalabalabeng reviewed Jul 15, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/ReadWorkbook.java Outdated

bengbengbalabalabeng reviewed Jul 15, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/FesodSheet.java

sapienza88 dismissed delei’s stale review via e6869c3 July 15, 2026 18:53

sapienza88 force-pushed the column_support branch from de14e8d to e6869c3 Compare July 15, 2026 18:53

address refactoring reviews

e6869c3

bengbengbalabalabeng reviewed Jul 16, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java Outdated

bengbengbalabalabeng reviewed Jul 16, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java Outdated

bengbengbalabalabeng reviewed Jul 16, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java Outdated

bengbengbalabalabeng reviewed Jul 16, 2026

View reviewed changes

Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/ReadWorkbook.java Outdated

sapienza88 force-pushed the column_support branch from b51a253 to fa2c984 Compare July 16, 2026 16:14

addressing reviews

fa2c984

delei requested a review from bengbengbalabalabeng July 17, 2026 11:33

bengbengbalabalabeng reviewed Jul 17, 2026

View reviewed changes

sapienza88 force-pushed the column_support branch from 93ec8a2 to cbd1291 Compare July 17, 2026 16:18

refactoring for columnIndex setting in cell

cbd1291

sapienza88 force-pushed the column_support branch from f4583f1 to 6379674 Compare July 19, 2026 10:26

Selim Soufargi and others added 2 commits July 19, 2026 12:28

fix setColumIndex value

6379674

Merge branch 'main' into column_support

d4176b0

bengbengbalabalabeng requested a review from delei July 19, 2026 12:46

bengbengbalabalabeng approved these changes Jul 20, 2026

View reviewed changes

Uh oh!

Conversation

sapienza88 commented Jul 5, 2026 • edited by bengbengbalabalabeng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

What's changed?

API Usage Example

Checklist

Uh oh!

delei commented Jul 5, 2026

Uh oh!

bengbengbalabalabeng commented Jul 5, 2026

Uh oh!

sapienza88 commented Jul 5, 2026

Uh oh!

bengbengbalabalabeng commented Jul 5, 2026

Uh oh!

delei commented Jul 5, 2026

Uh oh!

sapienza88 commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delei commented Jul 5, 2026

Uh oh!

sapienza88 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delei commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sapienza88 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sapienza88 commented Jul 9, 2026

Uh oh!

delei commented Jul 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapienza88 Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delei Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapienza88 Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delei Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapienza88 Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delei Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapienza88 Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapienza88 commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delei left a comment

Choose a reason for hiding this comment

Uh oh!

sapienza88 commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sapienza88 commented Jul 5, 2026 •

edited by bengbengbalabalabeng

Loading

sapienza88 commented Jul 5, 2026 •

edited

Loading

sapienza88 commented Jul 8, 2026 •

edited

Loading

delei commented Jul 9, 2026 •

edited

Loading

sapienza88 commented Jul 9, 2026 •

edited

Loading

sapienza88 Jul 9, 2026 •

edited

Loading

delei Jul 9, 2026 •

edited

Loading

sapienza88 Jul 11, 2026 •

edited

Loading

delei Jul 12, 2026 •

edited

Loading

sapienza88 Jul 12, 2026 •

edited

Loading

delei Jul 12, 2026 •

edited

Loading

sapienza88 Jul 12, 2026 •

edited

Loading

sapienza88 commented Jul 14, 2026 •

edited

Loading

sapienza88 commented Jul 15, 2026 •

edited

Loading

sapienza88 Jul 17, 2026 •

edited

Loading