----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25949/#review58735 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment99871> Would that be better to pass the TextFormatConfig instance to DrillTextRecordReader here? We won't update the constructor signature each time we add/remove/rename a property. exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment99868> Any particular reason for using dot notation instead of camel casing the JsonProperty names? It would be more consistent with the rest of config options across Drill if camel casing was preferred. exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment99869> can we reuse this whole code block for each and every setter that needs this functionality? exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java <https://reviews.apache.org/r/25949/#comment99870> Though this is not part of the change, did you have a chance to take a look at this? This does not seem right way to compute the hash code. exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java <https://reviews.apache.org/r/25949/#comment99894> we should remove @author exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java <https://reviews.apache.org/r/25949/#comment99896> In regards to the failing unit tests. Can we make sure all of the unit test suites are passing? - Hanifi Gunes 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 > >
