Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Pig, mail # dev - Review Request: disable optimizations via pig properties


+
Travis Crawford 2013-05-09, 21:03
+
Bill Graham 2013-05-09, 22:40
+
Travis Crawford 2013-05-10, 18:26
+
Travis Crawford 2013-05-13, 20:35
+
Travis Crawford 2013-05-13, 21:18
+
Bill Graham 2013-05-13, 23:35
+
Bill Graham 2013-05-14, 00:31
+
Travis Crawford 2013-05-14, 00:00
+
Bill Graham 2013-05-14, 15:00
+
Travis Crawford 2013-05-14, 02:19
+
Travis Crawford 2013-05-14, 03:45
+
Bill Graham 2013-05-14, 03:39
+
Julien Le Dem 2013-05-13, 23:23
+
Travis Crawford 2013-05-13, 23:50
+
Travis Crawford 2013-05-13, 23:47
Copy link to this message
-
Re: Review Request: disable optimizations via pig properties
Julien Le Dem 2013-05-14, 15:04


> 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.
+
Travis Crawford 2013-05-14, 17:23
+
Bill Graham 2013-05-14, 22:06
+
Julien Le Dem 2013-05-14, 23:13
+
Travis Crawford 2013-05-16, 00:07