> On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, > > line 70 > > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line70> > > > > 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.
An argument could be made either way. The fact that you are asking puts you in the same position as me. I only followed the model that was put in place. I'm personally indifferent and chose not to modify the model that was in place and only add the functionality. > On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, > > line 138 > > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line138> > > > > 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. The reason is that I couldn't find a single example of camel casing the json properties. I found plenty of examples in the drill json configuration files that used dots to separate instead of came case. > On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, > > line 140 > > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line140> > > > > can we reuse this whole code block for each and every setter that needs > > this functionality? You could if you want to. However the rules that are in place don't have to restrict to a single character. I used single character to respect what was already in place. If that changed then it would be potentially custom for each setter. > On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/TextFormatPlugin.java, > > line 219 > > <https://reviews.apache.org/r/25949/diff/1/?file=703157#file703157line219> > > > > 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. No I did not. I would consider this issue outside of the scope of this review. > On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java, > > line 27 > > <https://reviews.apache.org/r/25949/diff/1/?file=703160#file703160line27> > > > > In regards to the failing unit tests. Can we make sure all of the unit > > test suites are passing? The point of my comment on this review was to get someone else to validate if what I was seeing was on my machine / in my environment or not. > On Oct. 27, 2014, 8:48 p.m., Hanifi Gunes wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/store/easy/text/TextFormatConfigTest.java, > > line 25 > > <https://reviews.apache.org/r/25949/diff/1/?file=703160#file703160line25> > > > > we should remove @author If it is against a formatting rule to have an @author tag then it should be a part of the checkstyle rules or documented somewhere. I don't recall having seen that as a requirement to submitting functionality. - Jim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25949/#review58735 ----------------------------------------------------------- On Sept. 23, 2014, 2: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, 2: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 > >
