----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6884/#review11566 -----------------------------------------------------------
Have a few comments. contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java <https://reviews.apache.org/r/6884/#comment24871> Can these string constants be declared to make them reusable and more readable? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java <https://reviews.apache.org/r/6884/#comment24872> Is it multiple_schema or multiple_schemas ? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java <https://reviews.apache.org/r/6884/#comment25050> Use a descriptive variable name - path instead of p. contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java <https://reviews.apache.org/r/6884/#comment25052> Any reason for removing this here and not in other places? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorage.java <https://reviews.apache.org/r/6884/#comment25054> The logic used here is close to the logic used in setLocation. Is it possible to refactor the code and move this to a utility method? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25058> Why aren't float and double returning float and double respectively? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25059> Why aren't float and double returning float and double respectively? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25060> Why aren't int and long returning float ? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25061> Why aren't int and long returning double ? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25078> Can you add a more descriptive formal Javadoc? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25065> Is the equality applicable all the way through? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25062> Use a descriptive variable name - xField contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25064> Use a descriptive variable name - yField contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25063> Use a descriptive variable name - entry contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25066> Use a descriptive name - xSchema contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25067> Use a descriptive name - ySchema contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25068> Can you change the message in the exception to "Cannot merge FIXED types with different sizes:" contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25077> Can you add a more descriptive formal Javadoc? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25070> Can the name of the method be changed to getSchemaToMergedSchemaMap ? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25071> Use a descriptive variable name - entry? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25072> Use path contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/AvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25073> Use schema contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroInputFormat.java <https://reviews.apache.org/r/6884/#comment25080> Can you add a comment describing this data structure? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroInputFormat.java <https://reviews.apache.org/r/6884/#comment25082> Can this be removed or rephrased? Since this is a public method, it can be called by any method contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java <https://reviews.apache.org/r/6884/#comment25101> Can you add a comment on the variable(s) ? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java <https://reviews.apache.org/r/6884/#comment25103> This will append to the end of the list resulting in a list size of 2*tupleSize. Was that your intention? contrib/piggybank/java/src/main/java/org/apache/pig/piggybank/storage/avro/PigAvroRecordReader.java <https://reviews.apache.org/r/6884/#comment25121> Are the size of t and mProtoTuple expected to match? contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/6884/#comment25122> Should it be testMultipleSchemas1 ? contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorage.java <https://reviews.apache.org/r/6884/#comment25125> Should it be testMultipleSchemas2 ? contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25134> This test should be split into multiple tests contrib/piggybank/java/src/test/java/org/apache/pig/piggybank/test/storage/avro/TestAvroStorageUtils.java <https://reviews.apache.org/r/6884/#comment25131> Why isn't bytes + other non-null type = bytes ? - Santhosh Srinivasan On Sept. 13, 2012, 10:46 p.m., Cheolsoo Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6884/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2012, 10:46 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 > >
