----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14552/#review26894 -----------------------------------------------------------
Overall this looks good to me. Some comments regarding error handling in conf and defaults. trunk/src/org/apache/pig/backend/hadoop/executionengine/util/MapRedUtil.java <https://reviews.apache.org/r/14552/#comment52333> can we fix indentation and whitespace at the same time? - curly at end of line - spaces around operators - whitespace at end of line trunk/src/org/apache/pig/impl/util/Utils.java <https://reviews.apache.org/r/14552/#comment52335> We should throw an exception if the trimmed lowercased string is something we don't expect. otherwise typos (seqFile, sequencefile, ...) will lead to unexpected behavior. the call becomes then: ...getProperty(...PIG_TEMP_FILE_COMPRESSION_STORAGE, "tfile"); example message: PigConfiguration.PIG_TEMP_FILE_COMPRESSION_STORAGE +": expected storage seqfile or tfile (default tfile)" trunk/src/org/apache/pig/impl/util/Utils.java <https://reviews.apache.org/r/14552/#comment52336> duplicate logic? trunk/src/org/apache/pig/impl/util/Utils.java <https://reviews.apache.org/r/14552/#comment52339> you can use constants or enums for the available codecs. they could have fields for the corresponding codec class name and whether tfile supports them. trunk/src/org/apache/pig/impl/util/Utils.java <https://reviews.apache.org/r/14552/#comment52337> this is not consistent with the previous lines: else if (codec.equals("")) { ... } else { ... } trunk/src/org/apache/pig/impl/util/Utils.java <https://reviews.apache.org/r/14552/#comment52338> else throw exception? we have a default if it's not set but we don't do anything if it's an invalid compression codec - Julien Le Dem On Oct. 9, 2013, 6 p.m., Aniket Mokashi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14552/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2013, 6 p.m.) > > > Review request for pig, Cheolsoo Park, Dmitriy Ryaboy, Julien Le Dem, and > Rohini Palaniswamy. > > > Bugs: PIG-3480 > https://issues.apache.org/jira/browse/PIG-3480 > > > Repository: pig > > > Description > ------- > > - Added a new parameter to make SequenceFileInterStorage optional. > - Added tests > - Refactored apis > > > Diffs > ----- > > trunk/conf/pig.properties 1530468 > trunk/src/org/apache/pig/PigConfiguration.java 1530468 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java > 1530468 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/partitioners/WeightedRangePartitioner.java > 1530468 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartitionRearrange.java > 1530468 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/util/MapRedUtil.java > 1530468 > trunk/src/org/apache/pig/impl/io/InterStorage.java 1530468 > trunk/src/org/apache/pig/impl/io/SequenceFileInterStorage.java PRE-CREATION > trunk/src/org/apache/pig/impl/io/TFileStorage.java 1530468 > trunk/src/org/apache/pig/impl/util/Utils.java 1530468 > trunk/test/org/apache/pig/test/TestTmpFileCompression.java 1530468 > > Diff: https://reviews.apache.org/r/14552/diff/ > > > Testing > ------- > > > Thanks, > > Aniket Mokashi > >
