You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Santhosh Srinivasan <sm...@apache.org> on 2012/10/10 23:50:24 UTC
Re: Review Request: PIG-2579 Support for multiple input schemas in
AvroStorage
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6884/#review12319
-----------------------------------------------------------
Couple of comments. Otherwise, the patch looks good.
contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java
<https://reviews.apache.org/r/6884/#comment26091>
Can you replace add with set since you are already initializing the capacity of the array list?
contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java
<https://reviews.apache.org/r/6884/#comment26092>
Can you add a message for the failure?
- Santhosh Srinivasan
On Sept. 28, 2012, 8:44 p.m., Cheolsoo Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6884/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 8:44 p.m.)
>
>
> Review request for pig and Santhosh Srinivasan.
>
>
> Description
> -------
>
> Add support for multiple avro schemas to AvroStorage. This patch is based on Stan Rosenberg's original work.
>
> Please see https://issues.apache.org/jira/browse/PIG-2579 for details
>
>
> This addresses bug PIG-2579.
> https://issues.apache.org/jira/browse/PIG-2579
>
>
> Diffs
> -----
>
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java d7a004f
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java 84280af
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroInputFormat.java fb5cc25
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java 75057f9
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java 1f6e581
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java 0761d5a
>
> Diff: https://reviews.apache.org/r/6884/diff/
>
>
> Testing
> -------
>
> New unit tests are added:
> - TestAvroStorageUtils.testMergeSchema
> - TestAvroStorage.testMultipleSchemas1,2
>
>
> Thanks,
>
> Cheolsoo Park
>
>
Re: Review Request: PIG-2579 Support for multiple input schemas in
AvroStorage
Posted by Santhosh Srinivasan <sm...@apache.org>.
> On Oct. 10, 2012, 9:50 p.m., Santhosh Srinivasan wrote:
> > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java, line 102
> > <https://reviews.apache.org/r/6884/diff/3-4/?file=154467#file154467line102>
> >
> > Can you replace add with set since you are already initializing the capacity of the array list?
>
> Cheolsoo Park wrote:
> No, I can't because the size of list is still 0 even though I initialize its capacity until I add entries to it. Now set() throws an OutOfBoundException if (index < 0 || index >= size()), which is always true with any non-negative index as size() == 0.
>
> http://docs.oracle.com/javase/6/docs/api/java/util/ArrayList.html#set(int, E)
Thanks! My concerns around the performance are addressed.
- Santhosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6884/#review12319
-----------------------------------------------------------
On Oct. 10, 2012, 11:15 p.m., Cheolsoo Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6884/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2012, 11:15 p.m.)
>
>
> Review request for pig and Santhosh Srinivasan.
>
>
> Description
> -------
>
> Add support for multiple avro schemas to AvroStorage. This patch is based on Stan Rosenberg's original work.
>
> Please see https://issues.apache.org/jira/browse/PIG-2579 for details
>
>
> This addresses bug PIG-2579.
> https://issues.apache.org/jira/browse/PIG-2579
>
>
> Diffs
> -----
>
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java d7a004f
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java 84280af
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroInputFormat.java fb5cc25
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java 75057f9
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java 1f6e581
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java 0761d5a
>
> Diff: https://reviews.apache.org/r/6884/diff/
>
>
> Testing
> -------
>
> New unit tests are added:
> - TestAvroStorageUtils.testMergeSchema
> - TestAvroStorage.testMultipleSchemas1,2
>
>
> Thanks,
>
> Cheolsoo Park
>
>
Re: Review Request: PIG-2579 Support for multiple input schemas in
AvroStorage
Posted by Cheolsoo Park <ch...@cloudera.com>.
> On Oct. 10, 2012, 9:50 p.m., Santhosh Srinivasan wrote:
> > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java, line 102
> > <https://reviews.apache.org/r/6884/diff/3-4/?file=154467#file154467line102>
> >
> > Can you replace add with set since you are already initializing the capacity of the array list?
No, I can't because the size of list is still 0 even though I initialize its capacity until I add entries to it. Now set() throws an OutOfBoundException if (index < 0 || index >= size()), which is always true with any non-negative index as size() == 0.
http://docs.oracle.com/javase/6/docs/api/java/util/ArrayList.html#set(int, E)
- Cheolsoo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6884/#review12319
-----------------------------------------------------------
On Oct. 10, 2012, 11:15 p.m., Cheolsoo Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6884/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2012, 11:15 p.m.)
>
>
> Review request for pig and Santhosh Srinivasan.
>
>
> Description
> -------
>
> Add support for multiple avro schemas to AvroStorage. This patch is based on Stan Rosenberg's original work.
>
> Please see https://issues.apache.org/jira/browse/PIG-2579 for details
>
>
> This addresses bug PIG-2579.
> https://issues.apache.org/jira/browse/PIG-2579
>
>
> Diffs
> -----
>
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java d7a004f
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java 84280af
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroInputFormat.java fb5cc25
> contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java 75057f9
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java 1f6e581
> contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java 0761d5a
>
> Diff: https://reviews.apache.org/r/6884/diff/
>
>
> Testing
> -------
>
> New unit tests are added:
> - TestAvroStorageUtils.testMergeSchema
> - TestAvroStorage.testMultipleSchemas1,2
>
>
> Thanks,
>
> Cheolsoo Park
>
>