Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Pig >> mail # dev >> Review Request: PIG-3215 [piggybank] Add LTSVLoader to load LTSV files


+
Taku Miyakawa 2013-02-28, 22:53
Copy link to this message
-
Re: Review Request: PIG-3215 [piggybank] Add LTSVLoader to load LTSV files

-----------------------------------------------------------
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:
+
Cheolsoo Park 2013-03-04, 01:46
+
Taku Miyakawa 2013-03-03, 06:35
+
Jonathan Coveney 2013-03-04, 15:05
+
Taku Miyakawa 2013-03-05, 23:10
+
Jonathan Coveney 2013-03-19, 16:45
+
Taku Miyakawa 2013-03-05, 23:38
+
Taku Miyakawa 2013-03-05, 23:42
+
Taku Miyakawa 2013-03-07, 22:46
+
Taku Miyakawa 2013-03-07, 23:31
+
Taku Miyakawa 2013-03-09, 08:18