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-3141 [piggybank] Giving CSVExcelStorage an option to handle header rows


+
Jonathan Packer 2013-03-01, 14:51
+
Jonathan Packer 2013-03-01, 14:52
Copy link to this message
-
Re: Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to handle header rows
Cheolsoo Park 2013-03-20, 19:05

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9697/#review18100
-----------------------------------------------------------
Hi Jonathan,

Overall looks good to me. I made a few minor comment inline:
contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38336>

    This variable is no longer used.

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38195>

    Why don't you use "FIELD_DELIMITER_DEFAULT_STR" instead of "new String(new byte[] { (byte) ',' })"?

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38325>

    Can you remove 'DEFAULT' from the error message? I think it's unnecessary to let the user know about this string.

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38199>

    Can you log a warning message here?

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38340>

    Can you move this line to inside the if block? That's more intuitive to me.

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38326>

    Can you remove "modified from Pig 0.10 version" and just describe what's fixed here? e.g. "Substitute a null value with an empty string. See PIG-2470."
    
    When the code is modified can be easily found using "git blame", so it's unnecessary to make a comment about it.

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38327>

    Can you move this line to inside the if block? That's more intuitive to me.

contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38328>

    Can you remove "modified from Pig 0.10 version" and just describe what's fixed here?
    
    When the code gets modified can be easily found using "git blame", so it's unnecessary to make a comment about it.

contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38329>

    Can you remove this? This isn't a useful comment.

contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38330>

    Can you remove this?

contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38337>

    Can you remove this?

contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java
<https://reviews.apache.org/r/9697/#comment38217>

    Can you instead add the description to each test case? This isn't a useful comment.
- Cheolsoo Park
On March 1, 2013, 2:52 p.m., Jonathan Packer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9697/
> -----------------------------------------------------------
>
> (Updated March 1, 2013, 2:52 p.m.)
>
>
> Review request for pig.
>
>
> Description
> -------
>
> Reviewboard for https://issues.apache.org/jira/browse/PIG-3141
>
> Adds a "header treatment" option to CSVExcelStorage allowing header rows (first row with column names) in files to be skipped when loading, or for a header row with column names to be written when storing. Should be backwards compatible--all unit-tests from the old CSVExcelStorage pass.
>
>
> Diffs
> -
+
Cheolsoo Park 2013-03-20, 21:04
+
Jonathan Packer 2013-03-25, 15:17
+
Cheolsoo Park 2013-03-26, 04:13