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

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

This PR is for adding support for reading a sheet based on rows AND columns index.
Added the feature in the Sheet module and unit tests

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

bengbengbalabalabeng · 2026-07-15T10:23:18Z

+        List<Integer> includeColumnIndexes = null;
+        if (xlsxReadContext.readSheetHolder() != null


The null checks for xlsxReadContext.readSheetHolder() and getReadSheet() seem unnecessary here, The ReadSheetHolder is always initialized in

fesod/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java

Lines 317 to 319 in 68b7e1c

if (readSheet != null) {

try {

xlsxReadContext.currentSheet(readSheet);

Additionally, the filtering logic for columns can be moved to the beginning of the endElement method. For columns not in includeColumnIndexes, the corresponding ReadCellData instances do not need to be assigned.

bengbengbalabalabeng · 2026-07-15T10:23:23Z

+        this.sheetName = sheetName;
+        this.numRows = numRows;
+        this.columnIndexes = columnIndexes;
+    }


the numCols parameter is not used.

this.columnIndexes = columnIndexes; assigns the field to itself.

bengbengbalabalabeng · 2026-07-15T10:23:27Z

+    /**
+     * The number of columns to read, the default is all, start with 0.
+     */
+    private Integer numColumns;


The numColumns field is not used in the current change. Could you check whether this is intended?

bengbengbalabalabeng · 2026-07-15T10:23:34Z

+                .sheetNoIfNotNull(sheetNo)
+                .sheetNameIfNotNull(sheetName)
+                .numRowsIfNotNull(numRows)
+                .includeColumnIndexes(columnIndexes);


Is it more appropriate to replace includeColumnIndexes with includeColumnIndexesIfNotNull?

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 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

Merge branch 'main' into column_support

de14e8d

bengbengbalabalabeng reviewed Jul 15, 2026

View reviewed changes

		List<Integer> includeColumnIndexes = null;
		if (xlsxReadContext.readSheetHolder() != null

	if (readSheet != null) {
	try {
	xlsxReadContext.currentSheet(readSheet);

Uh oh!

Conversation

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!

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!

delei Jul 9, 2026

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!

sapienza88 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sapienza88 Jul 9, 2026

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!

bengbengbalabalabeng Jul 15, 2026

sapienza88 commented Jul 5, 2026 •

edited

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