Copilot commented on code in PR #693:
URL: https://github.com/apache/fesod/pull/693#discussion_r2533923871
##########
fesod/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellFormulaTagHandler.java:
##########
@@ -34,18 +42,89 @@ public class CellFormulaTagHandler extends
AbstractXlsxTagHandler {
public void startElement(XlsxReadContext xlsxReadContext, String name,
Attributes attributes) {
XlsxReadSheetHolder xlsxReadSheetHolder =
xlsxReadContext.xlsxReadSheetHolder();
xlsxReadSheetHolder.setTempFormula(new StringBuilder());
+
+ String formulaType =
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T);
+ String sharedIndex =
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_SI);
+
+ xlsxReadSheetHolder.setTempFormulaType(formulaType);
+ xlsxReadSheetHolder.setTempFormulaSharedIndex(sharedIndex != null ?
Integer.parseInt(sharedIndex) : null);
}
Review Comment:
Consider adding error handling for `Integer.parseInt(sharedIndex)`. If the
shared index attribute contains an invalid integer value, this will throw a
`NumberFormatException` that will propagate up the call stack. You should
either wrap this in a try-catch block or validate the value before parsing.
```suggestion
if (sharedIndex != null) {
try {
xlsxReadSheetHolder.setTempFormulaSharedIndex(Integer.parseInt(sharedIndex));
} catch (NumberFormatException e) {
// Optionally log the error or handle it as needed
xlsxReadSheetHolder.setTempFormulaSharedIndex(null);
}
} else {
xlsxReadSheetHolder.setTempFormulaSharedIndex(null);
}
```
##########
fesod/src/main/java/org/apache/fesod/sheet/read/metadata/holder/xlsx/XlsxReadSheetHolder.java:
##########
@@ -54,6 +56,18 @@ public class XlsxReadSheetHolder extends ReadSheetHolder {
* Formula for current label.
*/
private StringBuilder tempFormula;
+ /**
+ * Formula type for current label.
+ */
+ private String tempFormulaType;
+ /**
+ * Formula shared index for current label.
+ */
+ private Integer tempFormulaSharedIndex;
+ /**
+ * Map to store master shared formulas by their shared index.
+ */
+ private Map<Integer, SharedFormulaInfo> sharedFormulaMap;
Review Comment:
getSharedFormulaMap exposes the internal representation stored in field
sharedFormulaMap. The value may be modified [after this call to
getSharedFormulaMap](1).
##########
fesod/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellFormulaTagHandler.java:
##########
@@ -34,18 +42,89 @@ public class CellFormulaTagHandler extends
AbstractXlsxTagHandler {
public void startElement(XlsxReadContext xlsxReadContext, String name,
Attributes attributes) {
XlsxReadSheetHolder xlsxReadSheetHolder =
xlsxReadContext.xlsxReadSheetHolder();
xlsxReadSheetHolder.setTempFormula(new StringBuilder());
+
+ String formulaType =
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T);
+ String sharedIndex =
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_SI);
+
+ xlsxReadSheetHolder.setTempFormulaType(formulaType);
+ xlsxReadSheetHolder.setTempFormulaSharedIndex(sharedIndex != null ?
Integer.parseInt(sharedIndex) : null);
}
@Override
public void endElement(XlsxReadContext xlsxReadContext, String name) {
XlsxReadSheetHolder xlsxReadSheetHolder =
xlsxReadContext.xlsxReadSheetHolder();
+ String formulaText = xlsxReadSheetHolder.getTempFormula().toString();
+ String formulaType = xlsxReadSheetHolder.getTempFormulaType();
+ Integer sharedIndex = xlsxReadSheetHolder.getTempFormulaSharedIndex();
+
+ if (ExcelXmlConstants.ATTRIBUTE_SHARED.equals(formulaType) &&
sharedIndex != null) {
+ formulaText = handleSharedFormula(xlsxReadSheetHolder,
formulaText, sharedIndex);
+ }
+
FormulaData formulaData = new FormulaData();
-
formulaData.setFormulaValue(xlsxReadSheetHolder.getTempFormula().toString());
+ formulaData.setFormulaValue(formulaText);
xlsxReadSheetHolder.getTempCellData().setFormulaData(formulaData);
}
@Override
public void characters(XlsxReadContext xlsxReadContext, char[] ch, int
start, int length) {
xlsxReadContext.xlsxReadSheetHolder().getTempFormula().append(ch,
start, length);
}
+
+ /**
+ * Handles shared formula when reading from Excel
+ */
+ private String handleSharedFormula(XlsxReadSheetHolder
xlsxReadSheetHolder, String formulaText, int sharedIndex) {
+ Integer currentRow = xlsxReadSheetHolder.getRowIndex();
+ Integer currentCol = xlsxReadSheetHolder.getColumnIndex();
+
+ if (currentRow == null || currentCol == null) {
+ return formulaText;
+ }
+
+ // If formula text exists then this is the master cell
+ if (!StringUtils.isEmpty(formulaText)) {
+ xlsxReadSheetHolder
+ .getSharedFormulaMap()
+ .put(sharedIndex, new
XlsxReadSheetHolder.SharedFormulaInfo(formulaText, currentRow, currentCol));
+ return formulaText;
+ } else {
+ // No formula text means this is a shared reference cell
+ XlsxReadSheetHolder.SharedFormulaInfo masterInfo =
+ xlsxReadSheetHolder.getSharedFormulaMap().get(sharedIndex);
+
+ if (masterInfo == null) {
+ return "";
+ }
+
+ return convertSharedFormula(masterInfo, currentRow, currentCol);
+ }
+ }
+
+ /**
+ * Converts shared formula based on row and column offset
+ */
+ private String convertSharedFormula(
+ XlsxReadSheetHolder.SharedFormulaInfo masterInfo, int currentRow,
int currentCol) {
+ try {
+ // Parse the master formula text into tokens
+ Ptg[] masterPtgs =
FormulaParser.parse(masterInfo.getFormulaText(), null, FormulaType.CELL, 0);
+
+ // Calculate offset from the master cell position
+ int rowOffset = currentRow - masterInfo.getFirstRow();
+ int colOffset = currentCol - masterInfo.getFirstCol();
+
+ // Use POI SharedFormula to convert with offset
+ SharedFormula sharedFormula = new
SharedFormula(SpreadsheetVersion.EXCEL2007);
+ Ptg[] convertedPtgs =
sharedFormula.convertSharedFormulas(masterPtgs, rowOffset, colOffset);
+
+ // Convert tokens back to formula string
+ return FormulaRenderer.toFormulaString(null, convertedPtgs);
+ } catch (Exception e) {
+ // If conversion fails, return the master formula as-is
+ // This handles cases like volatile functions with no cell
references
+ // where the formula should be identical across all cells anyway
+ return masterInfo.getFormulaText();
+ }
Review Comment:
Silent exception swallowing can make debugging difficult. Consider adding
logging to record when formula conversion fails. This would help users
understand why they might be seeing unexpected master formulas instead of
properly converted cell-specific formulas. For example:
```java
} catch (Exception e) {
log.warn("Failed to convert shared formula at row {} col {}, using
master formula instead: {}",
currentRow, currentCol, masterInfo.getFormulaText(), e);
return masterInfo.getFormulaText();
}
```
##########
fesod/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellFormulaTagHandler.java:
##########
@@ -34,18 +42,89 @@ public class CellFormulaTagHandler extends
AbstractXlsxTagHandler {
public void startElement(XlsxReadContext xlsxReadContext, String name,
Attributes attributes) {
XlsxReadSheetHolder xlsxReadSheetHolder =
xlsxReadContext.xlsxReadSheetHolder();
xlsxReadSheetHolder.setTempFormula(new StringBuilder());
+
+ String formulaType =
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T);
+ String sharedIndex =
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_SI);
+
+ xlsxReadSheetHolder.setTempFormulaType(formulaType);
+ xlsxReadSheetHolder.setTempFormulaSharedIndex(sharedIndex != null ?
Integer.parseInt(sharedIndex) : null);
}
@Override
public void endElement(XlsxReadContext xlsxReadContext, String name) {
XlsxReadSheetHolder xlsxReadSheetHolder =
xlsxReadContext.xlsxReadSheetHolder();
+ String formulaText = xlsxReadSheetHolder.getTempFormula().toString();
+ String formulaType = xlsxReadSheetHolder.getTempFormulaType();
+ Integer sharedIndex = xlsxReadSheetHolder.getTempFormulaSharedIndex();
+
+ if (ExcelXmlConstants.ATTRIBUTE_SHARED.equals(formulaType) &&
sharedIndex != null) {
+ formulaText = handleSharedFormula(xlsxReadSheetHolder,
formulaText, sharedIndex);
+ }
+
FormulaData formulaData = new FormulaData();
-
formulaData.setFormulaValue(xlsxReadSheetHolder.getTempFormula().toString());
+ formulaData.setFormulaValue(formulaText);
xlsxReadSheetHolder.getTempCellData().setFormulaData(formulaData);
}
@Override
public void characters(XlsxReadContext xlsxReadContext, char[] ch, int
start, int length) {
xlsxReadContext.xlsxReadSheetHolder().getTempFormula().append(ch,
start, length);
}
+
+ /**
+ * Handles shared formula when reading from Excel
+ */
+ private String handleSharedFormula(XlsxReadSheetHolder
xlsxReadSheetHolder, String formulaText, int sharedIndex) {
+ Integer currentRow = xlsxReadSheetHolder.getRowIndex();
+ Integer currentCol = xlsxReadSheetHolder.getColumnIndex();
+
+ if (currentRow == null || currentCol == null) {
+ return formulaText;
+ }
+
+ // If formula text exists then this is the master cell
+ if (!StringUtils.isEmpty(formulaText)) {
+ xlsxReadSheetHolder
+ .getSharedFormulaMap()
+ .put(sharedIndex, new
XlsxReadSheetHolder.SharedFormulaInfo(formulaText, currentRow, currentCol));
+ return formulaText;
+ } else {
+ // No formula text means this is a shared reference cell
+ XlsxReadSheetHolder.SharedFormulaInfo masterInfo =
+ xlsxReadSheetHolder.getSharedFormulaMap().get(sharedIndex);
+
+ if (masterInfo == null) {
+ return "";
+ }
Review Comment:
Consider adding logging or a more informative return value when the master
formula is not found. Returning an empty string silently could make it
difficult to debug issues where:
1. A reference cell is processed before its master cell (unusual but
possible with malformed files)
2. The shared index doesn't match any existing master formula
For example:
```java
if (masterInfo == null) {
log.warn("Master formula not found for shared index {} at row {} col
{}",
sharedIndex, currentRow, currentCol);
return "";
}
```
--
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]
