hadoop-hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Edward Capriolo" <edlinuxg...@gmail.com>
Subject Re: Review Request: HIVE-1096: Hive Variables
Date Tue, 29 Jun 2010 14:20:10 GMT


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 260
> > <http://review.hbase.org/r/229/diff/1/?file=1601#file1601line260>
> >
> >     I like the word "interpolate" too, but I think more people are probably familiar
with "substitute". Please change the name to HIVESUBSTITUTEVARIABLES.

I do not want to have the feature called different things across the code base. Replace here
interpolate there it will be confusing for all. You originally suggested interpolate: "Driver.interpolateCommandVariables()".


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 561
> > <http://review.hbase.org/r/229/diff/1/?file=1601#file1601line561>
> >
> >     No need to test a boolean valued method for equality to true. The logic can
also be simplified as follows:
> >     
> >     if (!getBoolVar(ConfVars.HIVEVARIABLEINTERPOLATE)) {
> >       return expr;
> >     }
> >     l4j.info("Interpolation is on");
> >     ...
> >     
> >     Also, please move this out of the substitution function and into the Driver,
i.e Driver.compile() calls VariableSubstitution.substitute() iff HIVESUBSTITUEVARIABLES ==
true.

Different components are using substitute SetProcessor, Driver , File Processor, having the
interpolation on/off logic in each class is redundant. This way is better as it supports information
hiding.


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 562
> > <http://review.hbase.org/r/229/diff/1/?file=1601#file1601line562>
> >
> >     This log message is going to generate a lot of noise, and is unnecessary since
you can determine the value using the 'set' command. Please remove it.

I think it is very important to see the command before and after substitution. We can set
this to debug, I do not think it is noise.


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 587
> > <http://review.hbase.org/r/229/diff/1/?file=1601#file1601line587>
> >
> >     I thought the plan was to also introduce a "conf:" prefix for referencing configuration
properties, and that any value not prefixed by system/env/conf would map to a new variable
namespace? Please add the conf prefix and introduce some state in the SessionState for storing
variables (i.e. non System/Env/Conf properties).

After pondering this I think the "conf:" prefix is confusing and hurts backwards compatibility.
Right now this is "hive -hiveconf x=y"  = "set x=y". I do not think we want to introduce another
switch. "--hivevarconf". in most cases users want conf access to set properties that can be
picked up by classes. hadoop & hive conf is the way it is adding something else will not
fix the problem and will confuse people.


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/ql/src/test/queries/clientpositive/set_processor_namespaces.q, line 4
> > <http://review.hbase.org/r/229/diff/1/?file=1605#file1605line4>
> >
> >     You need a test for the "env" namespace. I think this is impossible to do here,
so you probably need to add a unit test.

Env variables are not changeable and are system dependent. I do not think there is a way to
test these.


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 595
> > <http://review.hbase.org/r/229/diff/1/?file=1601#file1601line595>
> >
> >     Please remove this log call. It will generate a lot of noise.

I think it is very important to see the command before and after substitution. We can set
this to debug, I do not think it is noise.


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 50
> > <http://review.hbase.org/r/229/diff/1/?file=1601#file1601line50>
> >
> >     The variables varPat and MAX_SUBST both exist as private variables in the parent
class. I think that redefining them in HiveConf will result in lots of confusion down the
road, plus they don't really belong here. Please move this to a "VariableSubstitution" class
located in ql.

I see your point. Then again, hadoop does the substitution inside the conf class. Also QL
is becoming the 'ubber package' At this point what isnt ql?


> On 2010-06-29 00:50:22, Carl Steinbach wrote:
> > trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java, line 93
> > <http://review.hbase.org/r/229/diff/1/?file=1604#file1604line93>
> >
> >     I don't think you want to perform variable substitution at this point. It makes
it impossible to create nested variables.

I will check it out.


- Edward


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/229/#review288
-----------------------------------------------------------


On 2010-06-23 20:02:57, Edward Capriolo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/229/
> -----------------------------------------------------------
> 
> (Updated 2010-06-23 20:02:57)
> 
> 
> Review request for Hive Developers.
> 
> 
> Summary
> -------
> 
> Hive Variables
> 
> 
> This addresses bug HIVE-1096.
>     http://issues.apache.org/jira/browse/HIVE-1096
> 
> 
> Diffs
> -----
> 
>   trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 955109 
>   trunk/conf/hive-default.xml 955109 
>   trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 955109 
>   trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java 955109 
>   trunk/ql/src/test/queries/clientpositive/set_processor_namespaces.q PRE-CREATION 
>   trunk/ql/src/test/results/clientpositive/set_processor_namespaces.q.out PRE-CREATION

> 
> Diff: http://review.hbase.org/r/229/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Edward
> 
>


Mime
View raw message