----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9685/#review17239 -----------------------------------------------------------
contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36628> In the case where they do not give it a schema, I think that they shouldn't have to actually specify that it is a map. It's implicit. Furthermore, I think that instead of passing the Schema as an argument, if they do an "as (schema)" THEN it should attempt to return a Schema in accordance with what you provide. Open to argument, though. contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36630> you don't reference this anywhere else, so I'd just inline the check as "if (line == null)" and so on. Java is verbose enough as it is :) contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36629> this isn't necessary -- it is impossible for this to be false contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36631> same comments as above...no use being more verbose than we need to be contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36632> I personally do not love the number of gratuitous newlines in the file, and would prefer it to be a bit more compact. I think it hinders readability and just spreads things out. This is a nitpick though :) contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36633> I prefer the pattern of having all of the members at the top of the file. Another style nitpick, but I think this class could use some unification in that respect (lord knows I've violated this in the past but I do think it makes things easier if there is a predictable place to look for things). contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36634> in Pig, all Maps are <String,Object> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36635> This should probably be LOG.debug contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36637> LOG.debug contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36636> same comments as above, and let's just apply it to every assert that comes right after a check which makes it clear what is desired. contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36638> Move this up to be with the null check ie "if (fields == null || fields.isEmpty())". It makes more sense for them to be together contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36639> LOG.debug contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36640> It is probably useful to LOG.debug (or LOG.warn) once for each label that comes up that isn't used. Right now it would be silent, and people might want to know. contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36644> Why doesn't this support pushProjection as well? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java <https://reviews.apache.org/r/9685/#comment36642> Spaces and tabs after the last character are discouraged contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestLTSVLoader.java <https://reviews.apache.org/r/9685/#comment36643> you should be able to just do new PigServer(ExecType.LOCAL) - Jonathan Coveney On Feb. 28, 2013, 10:53 p.m., Taku Miyakawa wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9685/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2013, 10:53 p.m.) > > > Review request for pig. > > > Description > ------- > > This is a review board for https://issues.apache.org/jira/browse/PIG-3215 > > The patch adds LTSVLoader function and its test class. > > > This addresses bug PIG-3215. > https://issues.apache.org/jira/browse/PIG-3215 > > > Diffs > ----- > > > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/LTSVLoader.java > PRE-CREATION > > contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestLTSVLoader.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/9685/diff/ > > > Testing > ------- > > ant compile-test > cd contrib/piggybank/java/ > ant -Dtestcase=TestLTSVLoader test > > > Thanks, > > Taku Miyakawa > >
