> 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 > >
