You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Jonathan Packer <jp...@mortardata.com> on 2013/03/01 15:51:22 UTC
Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to
handle header rows
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9697/
-----------------------------------------------------------
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
-----
contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java 568b3f3
contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java 9bed527
Diff: https://reviews.apache.org/r/9697/diff/
Testing
-------
Thanks,
Jonathan Packer
Re: Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to
handle header rows
Posted by Cheolsoo Park <pi...@gmail.com>.
> On March 20, 2013, 7:05 p.m., Cheolsoo Park wrote:
> > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java, line 538
> > <https://reviews.apache.org/r/9697/diff/2/?file=263987#file263987line538>
> >
> > Can you move this line to inside the if block? That's more intuitive to me.
Actually, I just realized that I was thinking wrong about this. Please ignore my comment.
> On March 20, 2013, 7:05 p.m., Cheolsoo Park wrote:
> > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java, line 442
> > <https://reviews.apache.org/r/9697/diff/2/?file=263987#file263987line442>
> >
> > Can you move this line to inside the if block? That's more intuitive to me.
Actually, I just realized that I was thinking wrong about this. Please ignore my comment.
- Cheolsoo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9697/#review18100
-----------------------------------------------------------
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
> -----
>
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java 568b3f3
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java 9bed527
>
> Diff: https://reviews.apache.org/r/9697/diff/
>
>
> Testing
> -------
>
> cd contrib/piggybank/java
> ant -Dtestcase=TestCSVExcelStorage test
>
>
> Thanks,
>
> Jonathan Packer
>
>
Re: Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to
handle header rows
Posted by Cheolsoo Park <pi...@gmail.com>.
-----------------------------------------------------------
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
> -----
>
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java 568b3f3
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java 9bed527
>
> Diff: https://reviews.apache.org/r/9697/diff/
>
>
> Testing
> -------
>
> cd contrib/piggybank/java
> ant -Dtestcase=TestCSVExcelStorage test
>
>
> Thanks,
>
> Jonathan Packer
>
>
Re: Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to
handle header rows
Posted by Cheolsoo Park <pi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9697/#review18379
-----------------------------------------------------------
Ship it!
Ship It!
- Cheolsoo Park
On March 25, 2013, 3:17 p.m., Jonathan Packer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9697/
> -----------------------------------------------------------
>
> (Updated March 25, 2013, 3:17 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
> -----
>
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java 568b3f3
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java 9bed527
>
> Diff: https://reviews.apache.org/r/9697/diff/
>
>
> Testing
> -------
>
> cd contrib/piggybank/java
> ant -Dtestcase=TestCSVExcelStorage test
>
>
> Thanks,
>
> Jonathan Packer
>
>
Re: Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to
handle header rows
Posted by Jonathan Packer <jp...@mortardata.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9697/
-----------------------------------------------------------
(Updated March 25, 2013, 3:17 p.m.)
Review request for pig.
Changes
-------
Code review changes for Cheolsoo
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 (updated)
-----
contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java 568b3f3
contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java 9bed527
Diff: https://reviews.apache.org/r/9697/diff/
Testing
-------
cd contrib/piggybank/java
ant -Dtestcase=TestCSVExcelStorage test
Thanks,
Jonathan Packer
Re: Review Request: PIG-3141 [piggybank] Giving CSVExcelStorage an option to
handle header rows
Posted by Jonathan Packer <jp...@mortardata.com>.
-----------------------------------------------------------
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
-----
contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/CSVExcelStorage.java 568b3f3
contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/TestCSVExcelStorage.java 9bed527
Diff: https://reviews.apache.org/r/9697/diff/
Testing (updated)
-------
cd contrib/piggybank/java
ant -Dtestcase=TestCSVExcelStorage test
Thanks,
Jonathan Packer