----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7336/#review12139 -----------------------------------------------------------
This overall looks pretty good. Is there a way you can avoid passing the PigContext around? Could you instead just pass the fields of the PigContext that you need to the SchemaTuple stuff? I would like to reduce the dependencies on PigContext. When you depend on PigContext, you depend on everything else. Thanks for refactoring the tests. I mentioned a few cases you missed. great stuff! src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POForEach.java <https://reviews.apache.org/r/7336/#comment25826> feel free to remove constructors that are not used anymore src/org/apache/pig/data/SchemaTupleBackend.java <https://reviews.apache.org/r/7336/#comment25827> why would you not throw an exception here? src/org/apache/pig/data/SchemaTupleFactory.java <https://reviews.apache.org/r/7336/#comment25828> is empty schema the same as no schema? src/org/apache/pig/data/SchemaTupleFrontend.java <https://reviews.apache.org/r/7336/#comment25829> why did you add this? src/org/apache/pig/newplan/logical/relational/LogToPhyTranslationVisitor.java <https://reviews.apache.org/r/7336/#comment25830> when would that happen? If we believe it can't happen let's throw an exception here. That's the best way to get it fixed. test/org/apache/pig/test/TestLoad.java <https://reviews.apache.org/r/7336/#comment25831> when using assertTrue you can pass a message to help debugging. Like op.getClass().getName() test/org/apache/pig/test/TestLoad.java <https://reviews.apache.org/r/7336/#comment25832> probably not what you want test/org/apache/pig/test/TestPigServer.java <https://reviews.apache.org/r/7336/#comment25833> assertTrue(url+" contain "+name, url.toString().contains(name)) test/org/apache/pig/test/TestPigServer.java <https://reviews.apache.org/r/7336/#comment25834> insert before the catch: fail("should have raised an exception") in the catch: assertTrue(e.getMessage(), e.getMessage().contains("<expected message>")) test/org/apache/pig/test/TestPigServer.java <https://reviews.apache.org/r/7336/#comment25835> just let the exception go through => remove the catch and add throws in the declaration test/org/apache/pig/test/TestPigServer.java <https://reviews.apache.org/r/7336/#comment25836> same test/org/apache/pig/test/TestPigServer.java <https://reviews.apache.org/r/7336/#comment25837> same test/org/apache/pig/test/TestPigServer.java <https://reviews.apache.org/r/7336/#comment25838> yes - Julien Le Dem On Sept. 28, 2012, 12:21 a.m., Jonathan Coveney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7336/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2012, 12:21 a.m.) > > > Review request for pig. > > > Description > ------- > > This uses the SchemaTuple in Foreach, which means it will work after Loads > > > This addresses bug PIG-2877. > https://issues.apache.org/jira/browse/PIG-2877 > > > Diffs > ----- > > src/org/apache/pig/PigConfiguration.java bf13d9d > src/org/apache/pig/PigConstants.java PRE-CREATION > src/org/apache/pig/backend/hadoop/executionengine/HExecutionEngine.java > d5c9315 > src/org/apache/pig/backend/hadoop/executionengine/HJob.java 2ff294a > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java > 86d04f8 > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigGenericMapBase.java > 6ab33a4 > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigGenericMapReduce.java > 2e70462 > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/partitioners/WeightedRangePartitioner.java > d7504ae > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/PhysicalOperator.java > 26d5389 > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POForEach.java > 4e6876a > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POSort.java > 702a01b > src/org/apache/pig/data/BinInterSedes.java a7ba39b > src/org/apache/pig/data/SchemaTuple.java 4f04834 > src/org/apache/pig/data/SchemaTupleBackend.java 2746316 > src/org/apache/pig/data/SchemaTupleClassGenerator.java ef15318 > src/org/apache/pig/data/SchemaTupleFactory.java b467d55 > src/org/apache/pig/data/SchemaTupleFrontend.java a8bfbab > src/org/apache/pig/impl/builtin/FindQuantiles.java 1fcbdb1 > src/org/apache/pig/impl/io/ReadToEndLoader.java afb6ebd > > src/org/apache/pig/newplan/logical/relational/LogToPhyTranslationVisitor.java > 5c3dc43 > src/org/apache/pig/parser/LogicalPlanBuilder.java b0a8aa0 > test/org/apache/pig/data/TestSchemaTuple.java 24142c2 > test/org/apache/pig/test/TestBuiltin.java a835993 > test/org/apache/pig/test/TestCommit.java 5baaedc > test/org/apache/pig/test/TestExampleGenerator.java 4babccb > test/org/apache/pig/test/TestLoad.java a34e2bb > test/org/apache/pig/test/TestPigServer.java 18a3dff > test/org/apache/pig/test/TestPigServerWithMacros.java c6d8c4e > > Diff: https://reviews.apache.org/r/7336/diff/ > > > Testing > ------- > > > Thanks, > > Jonathan Coveney > >
