> On May 13, 2013, 11:23 p.m., Julien Le Dem wrote: > > src/org/apache/pig/PigConstants.java, lines 47-51 > > <https://reviews.apache.org/r/11032/diff/2/?file=290927#file290927line47> > > > > I would like to clearly separate keys for pig.properties from others in > > there. > > > > Can we either move this one somewhere else, or somehow organize keys > > separately. This one is not used in pig.properties (as you mentioned in the > > doc). > > Travis Crawford wrote: > By move somewhere else, do you mean clearly comment in the code these are > internal constants, or move to a new org.apache.pig.impl.PigInternalConstants > class?
A new org.apache.pig.impl.PigInternalConstants class sounds good. That way there's a class documenting the API for users and a separate one for internals that does not need to clutter the public API. > On May 13, 2013, 11:23 p.m., Julien Le Dem wrote: > > src/org/apache/pig/newplan/logical/optimizer/LogicalPlanOptimizer.java, > > line 215 > > <https://reviews.apache.org/r/11032/diff/2/?file=290929#file290929line215> > > > > would checkNotNull(ruleSet, "ruleSet") give a better error message ? > > Travis Crawford wrote: > I'm generally against checkNotNull in constructors because an NPE > suggests the method did something wrong. checkArgument signals the caller did > something wrong. Since this is validating input I'd like to continue using > the latter. > > Travis Crawford wrote: > More specifically: > > - checkNotNull throws NullPointerException > - checkArgument throws IllegalArgumentException > > Even if the check ensures an argument is not null, I think the latter > exception more specifically signals why the error was thrown when checking an > argument. Sounds good. Then possibly use checkArgument with the message parameter? checkArgument(ruleSet != null, "ruleSet can not be null") - Julien ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11032/#review20514 ----------------------------------------------------------- On May 13, 2013, 8:35 p.m., Travis Crawford wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11032/ > ----------------------------------------------------------- > > (Updated May 13, 2013, 8:35 p.m.) > > > Review request for pig, Julien Le Dem, Bill Graham, and Feng Peng. > > > Description > ------- > > Update pig to allow disabling optimizations via pig properties. Currently > optimizations must be disabled via command-line options. Pig properties can > be set in pig.properties, "set" commands in scripts themselves, and > command-line -D options. > > The use-case is, for scripts that require certain optimizations to be > disabled, allowing the script itself to disable the optimization. Currently > whatever runs the script needs to specially handle disabling the optimization > for that specific query. > > > This addresses bug PIG-3317. > https://issues.apache.org/jira/browse/PIG-3317 > > > Diffs > ----- > > src/docs/src/documentation/content/xdocs/perf.xml 108ae7e > src/org/apache/pig/Main.java f97ed9f > src/org/apache/pig/PigConstants.java ea77e97 > src/org/apache/pig/backend/hadoop/executionengine/HExecutionEngine.java > 4dab4e8 > src/org/apache/pig/newplan/logical/optimizer/LogicalPlanOptimizer.java > d26f381 > test/org/apache/pig/test/TestEvalPipeline2.java 39cf807 > > Diff: https://reviews.apache.org/r/11032/diff/ > > > Testing > ------- > > Manually tested on a fully-distributed cluster. > > THIS FAILS: > PIG_CONF_DIR=/etc/pig/conf ./bin/pig -c query.pig > > THIS WORKS: > PIG_CONF_DIR=/etc/pig/conf ./bin/pig > -Dpig.optimizer.rules.disabled=ColumnMapKeyPrune -c query.pig > > Notice how "-Dpig.optimizer.rules.disabled=ColumnMapKeyPrune" specifies a pig > property, which could be in pig.properties, or the script itself. > > > Failure message: > > Pig Stack Trace > --------------- > ERROR 2229: Couldn't find matching uid -1 for project (Name: Project Type: > bytearray Uid: 97550 Input: 0 Column: 1) > > org.apache.pig.impl.logicalLayer.FrontendException: ERROR 1067: Unable to > explain alias null > at org.apache.pig.PigServer.explain(PigServer.java:1057) > at > org.apache.pig.tools.grunt.GruntParser.explainCurrentBatch(GruntParser.java:419) > at > org.apache.pig.tools.grunt.GruntParser.processExplain(GruntParser.java:351) > at org.apache.pig.tools.grunt.Grunt.checkScript(Grunt.java:98) > at org.apache.pig.Main.run(Main.java:607) > at org.apache.pig.Main.main(Main.java:152) > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) > at > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) > at java.lang.reflect.Method.invoke(Method.java:597) > at org.apache.hadoop.util.RunJar.main(RunJar.java:186) > Caused by: org.apache.pig.impl.logicalLayer.FrontendException: ERROR 2000: > Error processing rule ColumnMapKeyPrune. Try -t ColumnMapKeyPrune > at > org.apache.pig.newplan.optimizer.PlanOptimizer.optimize(PlanOptimizer.java:122) > at > org.apache.pig.backend.hadoop.executionengine.HExecutionEngine.compile(HExecutionEngine.java:281) > at org.apache.pig.PigServer.compilePp(PigServer.java:1380) > at org.apache.pig.PigServer.explain(PigServer.java:1042) > ... 10 more > Caused by: org.apache.pig.impl.logicalLayer.FrontendException: ERROR 2229: > Couldn't find matching uid -1 for project (Name: Project Type: bytearray Uid: > 97550 Input: 0 Column: 1) > at > org.apache.pig.newplan.logical.optimizer.ProjectionPatcher$ProjectionRewriter.visit(ProjectionPatcher.java:91) > at > org.apache.pig.newplan.logical.expression.ProjectExpression.accept(ProjectExpression.java:207) > at > org.apache.pig.newplan.DepthFirstWalker.depthFirst(DepthFirstWalker.java:64) > at > org.apache.pig.newplan.DepthFirstWalker.walk(DepthFirstWalker.java:53) > at org.apache.pig.newplan.PlanVisitor.visit(PlanVisitor.java:52) > at > org.apache.pig.newplan.logical.optimizer.AllExpressionVisitor.visit(AllExpressionVisitor.java:142) > at > org.apache.pig.newplan.logical.relational.LOInnerLoad.accept(LOInnerLoad.java:128) > at > org.apache.pig.newplan.DependencyOrderWalker.walk(DependencyOrderWalker.java:75) > at > org.apache.pig.newplan.logical.optimizer.AllExpressionVisitor.visit(AllExpressionVisitor.java:124) > at > org.apache.pig.newplan.logical.relational.LOForEach.accept(LOForEach.java:76) > at > org.apache.pig.newplan.DependencyOrderWalker.walk(DependencyOrderWalker.java:75) > at org.apache.pig.newplan.PlanVisitor.visit(PlanVisitor.java:52) > at > org.apache.pig.newplan.logical.optimizer.ProjectionPatcher.transformed(ProjectionPatcher.java:48) > at > org.apache.pig.newplan.optimizer.PlanOptimizer.optimize(PlanOptimizer.java:113) > ... 13 more > > > Thanks, > > Travis Crawford > >
