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 2019/12/24 20:09:00 UTC
[GitHub] [drill] cgivre opened a new pull request #1941: DRILL-7495: Excel
Reader Not Parsing Dates Correctly in First Column
cgivre opened a new pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941
This PR addresses a few minor bugs in the Excel Format Plugin in which dates were not being parsed correctly.
Also included in this PR are:
- A minor correction to the `README.md` document [](url)
- An update to the latest version of the Apache POI library.
[Jira link](https://issues.apache.org/jira/browse/DRILL-7495)
----------------------------------------------------------------
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 #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361338355
##########
File path: contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
##########
@@ -382,6 +382,26 @@ public void testExplicitSomeQueryWithCompressedFile() throws Exception {
new RowSetComparison(expected).verifyAndClearAll(results);
}
+ @Test
+ public void testFileWithDoubleDates() throws Exception {
+ String sql = "SELECT `Close Date`, `Type` FROM dfs.`excel/comps.xlsx` WHERE style='Contemporary'";
Review comment:
I removed this and incorporated the data into the one test file. However, the additional file was only 15kb. I understand that we want to keep Drill as lightweight as possible, but does 15kb really make that much of a difference?
----------------------------------------------------------------
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] arina-ielchiieva commented on a change in pull request
#1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361324464
##########
File path: contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
##########
@@ -382,6 +382,26 @@ public void testExplicitSomeQueryWithCompressedFile() throws Exception {
new RowSetComparison(expected).verifyAndClearAll(results);
}
+ @Test
+ public void testFileWithDoubleDates() throws Exception {
+ String sql = "SELECT `Close Date`, `Type` FROM dfs.`excel/comps.xlsx` WHERE style='Contemporary'";
+
+ RowSet results = client.queryBuilder().sql(sql).rowSet();
+ results.print();
Review comment:
Please remove. Printing pollutes the output.
----------------------------------------------------------------
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] KazydubB commented on a change in pull request #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r362183747
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -519,7 +519,10 @@ public void load(Cell cell) {
if (cellValue == null) {
columnWriter.setNull();
} else {
- Instant timeStamp = new Instant(cellValue.getNumberValue());
+ logger.debug("Cell value: {}", cellValue.getNumberValue());
+
+ Date dt = DateUtil.getJavaDate(cellValue.getNumberValue());
Review comment:
Thanks for the explanation, I think we are good then.
----------------------------------------------------------------
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] arina-ielchiieva commented on issue #1941: DRILL-7495:
Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-572587976
@cgivre since preparing batch commit branch takes time as well as running the tests we don't plan to do batch commits more often than once or twice a week so please be patient.
----------------------------------------------------------------
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] KazydubB commented on a change in pull request #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r362025701
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -412,7 +413,7 @@ private void addColumnToArray(TupleWriter rowWriter, String name, TypeProtos.Min
/**
* Returns the index of the first row of actual data. This function is to be used to find the header row as the POI skips blank rows.
- * @return: The headerRow index, corrected for blank rows
+ * @return int The headerRow index, corrected for blank rows
Review comment:
nit: there is no need to specify return type in `@return`, it looks awkward a little bit.
----------------------------------------------------------------
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 #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361452744
##########
File path: contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
##########
@@ -382,6 +382,26 @@ public void testExplicitSomeQueryWithCompressedFile() throws Exception {
new RowSetComparison(expected).verifyAndClearAll(results);
}
+ @Test
+ public void testFileWithDoubleDates() throws Exception {
+ String sql = "SELECT `Close Date`, `Type` FROM dfs.`excel/comps.xlsx` WHERE style='Contemporary'";
Review comment:
Ok. Noted for future. Thanks!
----------------------------------------------------------------
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] arina-ielchiieva commented on a change in pull request
#1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361324530
##########
File path: contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
##########
@@ -382,6 +382,26 @@ public void testExplicitSomeQueryWithCompressedFile() throws Exception {
new RowSetComparison(expected).verifyAndClearAll(results);
}
+ @Test
+ public void testFileWithDoubleDates() throws Exception {
+ String sql = "SELECT `Close Date`, `Type` FROM dfs.`excel/comps.xlsx` WHERE style='Contemporary'";
Review comment:
Do we need to add new file rather than updating existing ones? Project size matters...
----------------------------------------------------------------
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 #1941: DRILL-7495: Excel
Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-572580762
@cgivre, in future please **do not merge** pull requests which have code-changes without checking that regression tests are passed.
I was preparing the branch with commits to merge (it would include this one), but since you have merged the commit, I have to remove this commit from my branch and rebase it one more time.
Let's agree that in future if you see that your PR is not merged for a couple of weeks after it is approved, please ask someone who able to run regression tests to do that, and only after merge 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] KazydubB edited a comment on issue #1941: DRILL-7495: Excel
Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB edited a comment on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-570269960
@cgivre, while running tests I've encountered the next test failure:
```
[ERROR] Failures:
[ERROR] TestExcelFormat.testFileWithDoubleDates:401 1:0 expected:<1412308800000> but was:<1412294400000>
[INFO]
[ERROR] Tests run: 17, Failures: 1, Errors: 0, Skipped: 0
```
Probably, you need to use another `DateUtil.getJavaDate(...)` with `TimeZone` notion, rather then with system one.
----------------------------------------------------------------
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 #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r362130255
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -412,7 +413,7 @@ private void addColumnToArray(TupleWriter rowWriter, String name, TypeProtos.Min
/**
* Returns the index of the first row of actual data. This function is to be used to find the header row as the POI skips blank rows.
- * @return: The headerRow index, corrected for blank rows
+ * @return int The headerRow index, corrected for blank rows
Review comment:
Fixed
----------------------------------------------------------------
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 #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-571756051
Thanks. Will do!
> On Jan 7, 2020, at 3:24 PM, Arina Ielchiieva <no...@github.com> wrote:
>
> Should be good to go now, please squash the commits.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/drill/pull/1941?email_source=notifications&email_token=ABKB7PQSF6TK6DBROCKXDZTQ4TQJDA5CNFSM4J7ARU52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIKE25A#issuecomment-571755892>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKB7PXMJWDAMOXSQE55EFDQ4TQJDANCNFSM4J7ARU5Q>.
>
----------------------------------------------------------------
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] arina-ielchiieva commented on a change in pull request
#1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361451017
##########
File path: contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
##########
@@ -382,6 +382,26 @@ public void testExplicitSomeQueryWithCompressedFile() throws Exception {
new RowSetComparison(expected).verifyAndClearAll(results);
}
+ @Test
+ public void testFileWithDoubleDates() throws Exception {
+ String sql = "SELECT `Close Date`, `Type` FROM dfs.`excel/comps.xlsx` WHERE style='Contemporary'";
Review comment:
If we’ll look at this from the perspective that everybody will start adding files, this will increase project size.
Generally, we should rather than adding new files:
1. reuse existing files;
2. generate files at runtime, for example for excel files this could have been done fairly easy.
----------------------------------------------------------------
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] KazydubB commented on issue #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-570271066
@cgivre, excuse me, posted accidentaly while not finished :)
----------------------------------------------------------------
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 #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-570270213
Which test failure?
----------------------------------------------------------------
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] arina-ielchiieva commented on issue #1941: DRILL-7495:
Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-570512794
@cgivre travis does not run all the tests, CircleCI runs more tests but you have to enable it for your repo yourself, it is just in case if you need to run tests on the env different than your local.
----------------------------------------------------------------
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] KazydubB edited a comment on issue #1941: DRILL-7495: Excel
Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB edited a comment on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-570269960
@cgivre, while running tests I've encountered the next test failure:
```
[ERROR] Failures:
[ERROR] TestExcelFormat.testFileWithDoubleDates:401 1:0 expected:<1412308800000> but was:<1412294400000>
[INFO]
[ERROR] Tests run: 17, Failures: 1, Errors: 0, Skipped: 0
```
Probably, you need to use another `DateUtil.getJavaDate(...)` with `TimeZone` notion, and pass `UTC`, rather than using system time zone.
----------------------------------------------------------------
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 #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361338358
##########
File path: contrib/format-excel/src/test/resources/logback-test.xml
##########
@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="UTF-8" ?>
Review comment:
Removed.
----------------------------------------------------------------
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 #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r362129794
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -519,7 +519,10 @@ public void load(Cell cell) {
if (cellValue == null) {
columnWriter.setNull();
} else {
- Instant timeStamp = new Instant(cellValue.getNumberValue());
+ logger.debug("Cell value: {}", cellValue.getNumberValue());
+
+ Date dt = DateUtil.getJavaDate(cellValue.getNumberValue());
Review comment:
That was actually the source of the bug. I didn't realize it when I wrote the plugin but as it turns out, Excel generates all kinds of weird representations of dates. Specifically in the case dates (no time) it generates a `double` and when it has a time, it generates a `long`. Using the `DateUtil` class that is part of the POI library solves this problem and gets consistent dates out of Excel.
----------------------------------------------------------------
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 #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-571324793
@KazydubB Thanks for catching that. I found one more minor error which I fixed that affected reading large files.
----------------------------------------------------------------
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] arina-ielchiieva commented on issue #1941: DRILL-7495:
Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-571755892
Should be good to go now, please squash the 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] KazydubB commented on issue #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-570269960
@cgivre, while running tests I've encountered the next test failure:
----------------------------------------------------------------
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 merged pull request #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941
----------------------------------------------------------------
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 #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-572581167
My apologies. Won't do that again.
-- C
> On Jan 9, 2020, at 9:17 AM, Volodymyr Vysotskyi <no...@github.com> wrote:
>
> @cgivre <https://github.com/cgivre>, in future please do not merge pull requests which have code-changes without checking that regression tests are passed.
>
> I was preparing the branch with commits to merge (it would include this one), but since you have merged the commit, I have to remove this commit from my branch and rebase it one more time.
>
> Let's agree that in future if you see that your PR is not merged for a couple of weeks after it is approved, please ask someone who able to run regression tests to do that, and only after merge it.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/drill/pull/1941?email_source=notifications&email_token=ABKB7PWT7Y7Y5H4VEUNFLLLQ44WZJA5CNFSM4J7ARU52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIQOHGQ#issuecomment-572580762>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKB7PRVGUIHSSKABT2TIXDQ44WZJANCNFSM4J7ARU5Q>.
>
----------------------------------------------------------------
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] KazydubB commented on a change in pull request #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
KazydubB commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r362024925
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -519,7 +519,10 @@ public void load(Cell cell) {
if (cellValue == null) {
columnWriter.setNull();
} else {
- Instant timeStamp = new Instant(cellValue.getNumberValue());
+ logger.debug("Cell value: {}", cellValue.getNumberValue());
+
+ Date dt = DateUtil.getJavaDate(cellValue.getNumberValue());
Review comment:
Is it possible to use either `java.util` or `org.joda.time` classes?
----------------------------------------------------------------
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] arina-ielchiieva commented on issue #1941: DRILL-7495:
Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-569054996
+1, happy holidays to you, too :)
----------------------------------------------------------------
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] arina-ielchiieva commented on a change in pull request
#1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361324545
##########
File path: contrib/format-excel/src/test/resources/logback-test.xml
##########
@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="UTF-8" ?>
Review comment:
Please remove we have common logback-test.xml for all tests.
----------------------------------------------------------------
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 #1941:
DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#discussion_r361338312
##########
File path: contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
##########
@@ -382,6 +382,26 @@ public void testExplicitSomeQueryWithCompressedFile() throws Exception {
new RowSetComparison(expected).verifyAndClearAll(results);
}
+ @Test
+ public void testFileWithDoubleDates() throws Exception {
+ String sql = "SELECT `Close Date`, `Type` FROM dfs.`excel/comps.xlsx` WHERE style='Contemporary'";
+
+ RowSet results = client.queryBuilder().sql(sql).rowSet();
+ results.print();
Review comment:
Removed.
----------------------------------------------------------------
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 #1941: DRILL-7495: Excel Reader
Not Parsing Dates Correctly in First Column
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #1941: DRILL-7495: Excel Reader Not Parsing Dates Correctly in First Column
URL: https://github.com/apache/drill/pull/1941#issuecomment-568933123
@arina-ielchiieva
I addressed all review comments and squashed commits. Thanks for the review and Happy Holidays!
----------------------------------------------------------------
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