You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/12/01 13:54:38 UTC
[GitHub] [drill] cgivre opened a new pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
cgivre opened a new pull request #2118:
URL: https://github.com/apache/drill/pull/2118
# [DRILL-7813](https://issues.apache.org/jira/browse/DRILL-7813): Excel Reader Crashes on Formulae Which Return Strings
## Description
When querying Excel files, if there is a column which had a formula that returned text, the reader would throw a `NumberFormatException` when trying to parse the result. This minor bug fix checks the type of the result to see whether if it is numeric or not, and maps it to the correct column type.
## Documentation
No user visible changes other than no errors when querying. :-)
## Testing
Added two unit tests to test for text and numeric formulaic columns.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre commented on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736611390
@dzamo
Thanks for the review. I made the change you requested. Could leave a `+1` if you're ok with the changes?
Thx!
----------------------------------------------------------------
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
[GitHub] [drill] vvysotskyi commented on a change in pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by 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
[GitHub] [drill] cgivre edited a comment on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre edited a comment on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736608577
> My notes
>
> * The new test code covers both the text-valued formula and number-valued formula cases.
> * I looked for some simple, fast alternatives to the exception-driven testing of whether a cell's content can be parsed as a double but I found nothing compelling.
I didn't either. Unfortunately, the streaming reader does place some limits on what is possible.
> * On line 467 of ExcelBatchReader.java `numericValue` is a throw-away. Possibly a style improvement is not to assign the value of `cell.getNumericCellValue()` to anything, or to assign it to something explicit like `double throwAway = ...`
>
> In ExcelBatchReader.java
I'll make that change. Really all we want to do is see whether it CAN be read. We don't actually use the value at that point.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre commented on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736758340
Thanks @vvysotskyi for the review. Commits squashed and I'll merge this tomorrow.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre edited a comment on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre edited a comment on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736608577
> My notes
>
> * The new test code covers both the text-valued formula and number-valued formula cases.
> * I looked for some simple, fast alternatives to the exception-driven testing of whether a cell's content can be parsed as a double but I found nothing compelling.
I didn't either. Unfortunately, the streaming reader does place some limits on what is possible.
> * On line 467 of ExcelBatchReader.java `numericValue` is a throw-away. Possibly a style improvement is not to assign the value of `cell.getNumericCellValue()` to anything, or to assign it to something explicit like `double throwAway = ...`
>
> In ExcelBatchReader.java
I'll make that change. Really all we want to do is see whether it CAN be read. We don't actually use the value at that point. Also, just FYSA, that code is only executed once per batch, so it isn't a huge performance loss.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre commented on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736612109
Commits have been squashed.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre commented on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736608577
> My notes
>
> * The new test code covers both the text-valued formula and number-valued formula cases.
> * I looked for some simple, fast alternatives to the exception-driven testing of whether a cell's content can be parsed as a double but I found nothing compelling.
I didn't either unfortunately. Unfortunately, the streaming reader does place some limits on what is possible.
> * On line 467 of ExcelBatchReader.java `numericValue` is a throw-away. Possibly a style improvement is not to assign the value of `cell.getNumericCellValue()` to anything, or to assign it to something explicit like `double throwAway = ...`
>
> In ExcelBatchReader.java
I'll make that change. Really all we want to do is see whether it CAN be read. We don't actually use the value at that point.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre commented on pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2118:
URL: https://github.com/apache/drill/pull/2118#issuecomment-736749425
@vvysotskyi
I actually figured out another method for doing this that seems to work.
----------------------------------------------------------------
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
[GitHub] [drill] cgivre merged pull request #2118: Drill-7813: Excel Reader Crashes on Formulae Which Return Strings
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #2118:
URL: https://github.com/apache/drill/pull/2118
----------------------------------------------------------------
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