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/02/20 16:46:03 UTC
[GitHub] [drill] cgivre opened a new pull request #1991: DRILL-7514: Update
Apache POI to Latest Version
cgivre opened a new pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991
# [DRILL-7514](https://issues.apache.org/jira/browse/DRILL-7514): Update Apache POI to Latest Version
## Description
Drill's Excel Format Plugin uses Apache POI to parse Excel files. While this reader is effective in that it parses formulae and data types, it uses memory inefficiently and will struggle to read very large Excel files.
The latest version of POI addresses some of the memory issues and hopefully Drill will be able to query larger Excel files without running out of memory.
This PR updates Drill to use the latest version of Apache POI and also updates the User Agent Parser to a more recent version.
There was a minor change to the POI's behavior with respect to empty sheets, so I had to enclose a line in a `try/catch` block.
## Documentation
No user visible changes.
## Testing
All relevant unit tests run and passed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1991:
DRILL-7514: Update Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#discussion_r382172076
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -273,7 +273,12 @@ private int getColumnCount() {
int columnCount;
if (readerConfig.headerRow >= 0) {
- columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ try {
+ columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ } catch (NullPointerException e) {
Review comment:
@vvysotskyi
Thanks for the review. I replaced the method with your code and all unit tests pass.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1991:
DRILL-7514: Update Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#discussion_r382168457
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -273,7 +273,12 @@ private int getColumnCount() {
int columnCount;
if (readerConfig.headerRow >= 0) {
- columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ try {
+ columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ } catch (NullPointerException e) {
Review comment:
Thanks for fixing it, but I'm afraid we can obtain NPE here for other cases, according to its JavaDoc, null may be returned for the case when a row with specified index wasn't defined. Also, there is a line of code below which may have a possible issue. I would recommend refactoring it to something like this:
```
private int getColumnCount() {
int rowNumber = readerConfig.headerRow > 0 ? sheet.getFirstRowNum() : 0;
XSSFRow sheetRow = sheet.getRow(rowNumber);
return sheetRow != null ? sheetRow.getPhysicalNumberOfCells() : 0;
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1991:
DRILL-7514: Update Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#discussion_r382134743
##########
File path: contrib/udfs/pom.xml
##########
@@ -66,7 +66,7 @@
<dependency>
<groupId>nl.basjes.parse.useragent</groupId>
<artifactId>yauaa</artifactId>
- <version>5.11</version>
+ <version>5.13</version>
Review comment:
There is `5.15` version of this lib in maven central, should we use it?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1991:
DRILL-7514: Update Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#discussion_r382162056
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -273,7 +273,12 @@ private int getColumnCount() {
int columnCount;
if (readerConfig.headerRow >= 0) {
- columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ try {
+ columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ } catch (NullPointerException e) {
Review comment:
Fixed. If the `sheet.getFirstRowNum())` is `-1` then the sheet is empty. I included a check for this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on issue #1991: DRILL-7514: Update Apache
POI to Latest Version
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#issuecomment-589236855
@vvysotskyi Thanks for the review. Commits 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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on issue #1991: DRILL-7514: Update Apache
POI to Latest Version
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#issuecomment-589230233
@vvysotskyi
Addressed review comments and squashed commits.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on issue #1991: DRILL-7514: Update
Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on issue #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#issuecomment-589231786
@cgivre, please address this comment: https://github.com/apache/drill/pull/1991#discussion_r382168457
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1991:
DRILL-7514: Update Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#discussion_r382132759
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -273,7 +273,12 @@ private int getColumnCount() {
int columnCount;
if (readerConfig.headerRow >= 0) {
- columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ try {
+ columnCount = sheet.getRow(sheet.getFirstRowNum()).getPhysicalNumberOfCells();
+ } catch (NullPointerException e) {
Review comment:
It is a bad practice to catch NPE instead of adding a check for null. If I understand the API correctly, `sheet.getRow(sheet.getFirstRowNum())` may return null for the case when row is not defined. Could you please add check for null there?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1991:
DRILL-7514: Update Apache POI to Latest Version
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991#discussion_r382159258
##########
File path: contrib/udfs/pom.xml
##########
@@ -66,7 +66,7 @@
<dependency>
<groupId>nl.basjes.parse.useragent</groupId>
<artifactId>yauaa</artifactId>
- <version>5.11</version>
+ <version>5.13</version>
Review comment:
Unfortunately versions 5.14 and 5.15 use a banned dependency which causes problems when you exclude it. I'm going to work with the developer to see if we can fix 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
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #1991: DRILL-7514: Update Apache
POI to Latest Version
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1991: DRILL-7514: Update Apache POI to Latest Version
URL: https://github.com/apache/drill/pull/1991
----------------------------------------------------------------
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
With regards,
Apache Git Services