----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5936/#review9293 -----------------------------------------------------------
Some more comments. contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/5936/#comment19908> Looks like this variable is not used. contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/5936/#comment19907> escaped contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/5936/#comment19906> escaped contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/5936/#comment19905> escaped contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/5936/#comment19904> escaped contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/5936/#comment19901> Can you add an assert here that checks for the expected message from the exception? contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java <https://reviews.apache.org/r/5936/#comment19903> Can you add an assert to check for the message from the exception? contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java <https://reviews.apache.org/r/5936/#comment19902> Why is this check required? What are the guarantees for concretePath being null. There is an assumption here that concretePath is/was null. It could be due to the previous test setting concretePath to null. - Santhosh Srinivasan On July 19, 2012, 1:23 a.m., Cheolsoo Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5936/ > ----------------------------------------------------------- > > (Updated July 19, 2012, 1:23 a.m.) > > > Review request for pig. > > > Description > ------- > > Add glob support to AvroStorage: > > https://issues.apache.org/jira/browse/PIG-2492 > > > This addresses bug PIG-2492. > https://issues.apache.org/jira/browse/PIG-2492 > > > Diffs > ----- > > > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java > 0f8ef27 > > contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java > c7de726 > > contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java > 48b093b > > contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java > e5d0c38 > > Diff: https://reviews.apache.org/r/5936/diff/ > > > Testing > ------- > > 1. Added new unit tests as follows: > > - testDir verifies that AvroStorage recursively loads files in a directory > and its sub-directories. > - testGlob1 to 3 verify that glob patterns are expanded properly. > > To run the tests, please do the following: > > wget > https://issues.apache.org/jira/secure/attachment/12536534/avro_test_files.tar.gz > > tar -xf avro_test_files.tar.gz > ant clean compile-test piggybank -Dhadoopversion=20 > cd contrib/piggybank/java > ant test -Dtestcase=TestAvroStorage > > 2. Both TestAvroStorage and TestAvroStorageUtils pass. > > > Thanks, > > Cheolsoo Park > >
