bengbengbalabalabeng commented on code in PR #942:
URL: https://github.com/apache/fesod/pull/942#discussion_r3586419257
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java:
##########
@@ -134,6 +135,22 @@ public void endElement(XlsxReadContext xlsxReadContext,
String name) {
tempCellData.checkEmpty();
tempCellData.setRowIndex(xlsxReadSheetHolder.getRowIndex());
tempCellData.setColumnIndex(xlsxReadSheetHolder.getColumnIndex());
-
xlsxReadSheetHolder.getCellMap().put(xlsxReadSheetHolder.getColumnIndex(),
tempCellData);
+ List<Integer> includeColumnIndexes = null;
+ if (xlsxReadContext.readSheetHolder() != null
Review Comment:
The null checks for `xlsxReadContext.readSheetHolder()` and `getReadSheet()`
seem unnecessary here, The `ReadSheetHolder` is always initialized in
https://github.com/apache/fesod/blob/68b7e1c81f18ad52f26dfaca14763b8d58dfee3c/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java#L317-L319
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.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/ReadSheet.java:
##########
@@ -71,6 +77,13 @@ public ReadSheet(Integer sheetNo, String sheetName, Integer
numRows) {
this.numRows = numRows;
}
+ public ReadSheet(Integer sheetNo, String sheetName, Integer numRows,
Integer numCols) {
+ this.sheetNo = sheetNo;
+ this.sheetName = sheetName;
+ this.numRows = numRows;
+ this.columnIndexes = columnIndexes;
+ }
Review Comment:
- the `numCols` parameter is not used.
- `this.columnIndexes = columnIndexes;` assigns the field to itself.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/read/metadata/ReadWorkbook.java:
##########
@@ -144,6 +144,11 @@ public class ReadWorkbook extends ReadBasicParameter {
*/
private Integer numRows;
+ /**
+ * The number of columns to read, the default is all, start with 0.
+ */
+ private Integer numColumns;
Review Comment:
The `numColumns` field is not used in the current change. Could you check
whether this is intended?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
