----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1817/#review1877 -----------------------------------------------------------
trunk/src/org/apache/pig/Main.java <https://reviews.apache.org/r/1817/#comment4318> will do. I will set the properties to the default value (if any), and ask the user to use "-h properties" command for description of the properties. (I don't want to increase the sources of truth for the details of the properties). trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/CombinerOptimizer.java <https://reviews.apache.org/r/1817/#comment4319> Will do. For future - it might be a good idea to generate these settings for pig code conventions as well with the eclipse-files target. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4320> fixing! trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4336> will do some refactoring. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4322> Yes, the mapDumpIteral will be non null if there were accumulated entries in the map. Will move the disableMapAgg out to improve readability. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4324> The return type is a Result object, STATUS_EOP is a byte. I don't want to create a new object. It might be useful to create 'static final' Result objects for these cases to improve readability. (Not doing that as part of this patch.) trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4325> described in comment where valueTuple is defined. // tuple of the format - (null(key),bag-val1,bag-val2,...) // attach this to the plans with algebraic udf before evaluating the plans private transient Tuple valueTuple = null; The combiner plan is used by POPartialAgg without resetting the input offsets, that is why the first field has the key. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4326> changing. I thought a check was being done at pig query compile time, but apparently thats not the case. Created- PIG-2283 trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4327> existingResult is the tuple that holds key and value(s), if the key is present in aggMap, it will never be null. I didn't want to make two hashmap function calls (.contains() + .get()). I am adding a comment to clarify. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4328> this is the output of partial aggregation. it is the result of getOutput() function, which returns only ERR_RESULT or Result with STATUS_OK. Getting a status such as STATUS_NULL ,STATUS_EOP or STATUS_BATCH_OK does not make sense, and should result in an error (the query fails). trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4329> changing. i have re-formatted this whole file using eclipse, and its fixed all this. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4330> Thanks for the suggesting it, that seems to be a good candidate. But I think that it is something that is better explored in a separate jira. It certainly needs a benchmark run. It might be important to select more 'realistic' data to see how useful LRU/LFU replacement is going to be. I am not sure if pigmix is representative of data where LRU/LFU will be useful. The memory footprint of LinkedHashMap would be higher. The memory usage estimation logic also would need to change as it is based on sizes for HashMap. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4331> yeah, we do have large screens with awesome resolution ;) But I will fix the excess newlines for the less fortunate :) trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4332> will create a public static final variable in POPartAgg for the property. But I think it would be good to define it in some central place with other properties. But we don't have such a place now. Will also use getInt. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4333> changing. trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java <https://reviews.apache.org/r/1817/#comment4334> the cast here is actually unnecessary, removing it. - Thejas On 2011-09-12 23:55:12, Thejas Nair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1817/ > ----------------------------------------------------------- > > (Updated 2011-09-12 23:55:12) > > > Review request for pig, Daniel Dai and Dmitriy Ryaboy. > > > Summary > ------- > > See PIG-2228 > > > This addresses bug PIG-2228. > https://issues.apache.org/jira/browse/PIG-2228 > > > Diffs > ----- > > trunk/src/org/apache/pig/Algebraic.java 1164722 > trunk/src/org/apache/pig/Main.java 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/CombinerOptimizer.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceLauncher.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PhyPlanSetter.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/plans/EndOfAllInputSetter.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/PhyPlanVisitor.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/PlanPrinter.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POLocalRearrange.java > 1164722 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartialAgg.java > PRE-CREATION > trunk/src/org/apache/pig/data/DefaultTuple.java 1164722 > trunk/src/org/apache/pig/data/InternalCachedBag.java 1164722 > trunk/src/org/apache/pig/data/InternalDistinctBag.java 1164722 > trunk/src/org/apache/pig/data/InternalSortedBag.java 1164722 > trunk/src/org/apache/pig/data/SelfSpillBag.java PRE-CREATION > trunk/src/org/apache/pig/data/SizeUtil.java PRE-CREATION > trunk/src/org/apache/pig/data/SortedSpillBag.java 1164722 > trunk/test/e2e/pig/tests/nightly.conf 1164722 > trunk/test/org/apache/pig/test/TestDataBag.java 1164722 > trunk/test/org/apache/pig/test/TestPOPartialAgg.java PRE-CREATION > trunk/test/org/apache/pig/test/TestPOPartialAggPlan.java PRE-CREATION > trunk/test/org/apache/pig/test/Util.java 1164722 > trunk/test/org/apache/pig/test/utils/GenPhyOp.java 1164722 > > Diff: https://reviews.apache.org/r/1817/diff > > > Testing > ------- > > test-patch > [exec] -1 overall. > [exec] > [exec] +1 @author. The patch does not contain any @author tags. > [exec] > [exec] +1 tests included. The patch appears to include 21 new or > modified tests. > [exec] > [exec] +1 javadoc. The javadoc tool did not generate any warning > messages. > [exec] > [exec] +1 javac. The applied patch does not increase the total > number of javac compiler warnings. > [exec] > [exec] +1 findbugs. The patch does not introduce any new Findbugs > warnings. > [exec] > [exec] -1 release audit. The applied patch generated 461 release > audit warnings (more than the trunk's current 455 warnings). > release audit failures are because of jdiff changes > > All unit tests pass, new e2e tests added . > > > Thanks, > > Thejas > >
