----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25949/#review59545 -----------------------------------------------------------
Your note at the top about failing tests. Did the master commit before your commit work? Otherwise, small fixes. Try to get this fixed tomorrow so we can get this in early this week. exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment100803> Please just clean this up so you aren't doing a cast over and over again. The other issue is fine as is. exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment100804> we use dashes to describe a multiword. such as field-separator exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment100805> Yes, please refactor this out as a utility method. E.g. getCharacter(String value, String error message, char defaultValue) and similar if there are variations. exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment100808> remove comment. exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment100809> this seems wrong. Shouldn't these be equivalent sets? exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java <https://reviews.apache.org/r/25949/#comment100806> why wouldn't you just use a byte in the previous position. You should be consistent. exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java <https://reviews.apache.org/r/25949/#comment100807> Why are you jumping back to string here? Why drop to char/byte and then come back? exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java <https://reviews.apache.org/r/25949/#comment100811> Yes, it is against the rules. Same for all Apache products. Feel free to open a separate JIRA to add checkstyle rules and submit a patch. exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java <https://reviews.apache.org/r/25949/#comment100813> Once you get your updates done, please request this on the mailing list and someone will run the suite to verify that your system is borked. In general, you should assume that the test suite runs to completion. - Jacques Nadeau On Sept. 23, 2014, 7:53 p.m., Jim Scott wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25949/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2014, 7:53 p.m.) > > > Review request for drill. > > > Repository: drill-git > > > Description > ------- > > Updates for handling text formatted files to address: > https://issues.apache.org/jira/browse/DRILL-1440 > > > Diffs > ----- > > exec/java-exec/pom.xml 81dbeff > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java > 76c4ace > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java > b64a032 > > exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java > 7b8761c > > exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java > 31b1fbe > > exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/25949/diff/ > > > Testing > ------- > > There is a test class added to the serialization json work of the data > formats including maintaing backward compatibility as well as altering the > field separator name. > > Maven tests were run, with these two errors: > TestWriter.simpleCsv » org.apache.drill.exec.rpc.RpcException: Failure > while running fragment. null [479d0e71-2bad-4f4c-b3fc-8bffc98f788b] > TestWriter>BaseTestQuery.closeClient:125 » java.lang.IllegalStateException: > Failure while trying to close allocator: Child level allocators not closed. > > I could not figure out the cause for these failures, nor could I properly > debug why they were occurring. The code in the patch should have no impact on > those tests (although, I could be wrong). > > > Thanks, > > Jim Scott > >
