[GitHub] [drill] vvysotskyi commented on a change in pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings

[GitBox <gi...@apache.org>]

vvysotskyi commented on a change in pull request #2118:
URL: https://github.com/apache/drill/pull/2118#discussion_r533521686



##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -459,7 +459,18 @@ private void populateColumnArray(Cell cell, int colPosition) {
     } else if (cellType == CellType.NUMERIC && DateUtil.isCellDateFormatted(cell)) {
       // Case if the column is a date or time
       addColumnToArray(rowWriter, excelFieldNames.get(colPosition), MinorType.TIMESTAMP, false);
-    } else if (cellType == CellType.NUMERIC || cellType == CellType.FORMULA || cellType == CellType.BLANK || cellType == CellType._NONE) {
+    } else if (cellType == CellType.FORMULA) {
+      // Cells with formulae can return either strings or numbers.  Unfortunately, there is no way to
+      // determine what he result of the formula is, so we check to see if the result CAN be converted to a
+      // number.  If yes, we assume the cell is numeric.  If not, we catch the error and write the results as strings
+      try {
+        // This tests to see whether the cell can be read as a number. The return value is not needed here.
+        cell.getNumericCellValue();

Review comment:
       Can we use here something like `FormulaEvaluator` to evaluate the result of the formula before obtaining the cell type? If I understand correctly, `FormulaEvaluator` replaces formula with its value, so cell type should be correct after that




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org