----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/229/#review288 -----------------------------------------------------------
trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1289> This code does not belong in common. Please move it to ql. trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1290> 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. trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1291> I like the word "interpolate" too, but I think more people are probably familiar with "substitute". Please change the name to HIVESUBSTITUTEVARIABLES. trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1292> Please fix the formatting problems in this method (spaces before and after parens, no space between literals and operators, etc). Please run checkstyle and verify that you are not introducing any new checkstyle errors. trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1293> 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. trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1294> 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. trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1295> Presumably there aren't going to be any properties in the HiveConf starting with "system:" or "env:". Please move the "env:" check before the conf check, and move these string comparisons outside of the try/catch blocks, e.g: private static final String SYSTEM_VAR_PREFIX = "system:"; private static final String ENV_VAR_PREFIX = "env:"; if (var.startsWith(SYSTEM_VAR_PREFIX)) { try { val = System.getProperty(var.substring(SYSTEM_VAR_PREFIX.length())); } catch (SecurityException se) { ... } } else if (var.startsWith(ENV_VAR_PREFIX)) { val = System.getenv(var.substring(ENV_VAR_PREFIX.length())); } else { val = ss.getConf().get(var); } trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1303> 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). trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <http://review.hbase.org/r/229/#comment1296> Please remove this log call. It will generate a lot of noise. trunk/conf/hive-default.xml <http://review.hbase.org/r/229/#comment1297> Please change the name to "hive.substitute.variables", and the description to "Enable variable substitution". trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java <http://review.hbase.org/r/229/#comment1298> Check HIVESUBSTITUTEVARIABLES here. trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java <http://review.hbase.org/r/229/#comment1299> Please create static final variables for "system:" and "env:" and call length() on these variables instead of using magic numbers. trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java <http://review.hbase.org/r/229/#comment1300> I don't think you want to perform variable substitution at this point. It makes it impossible to create nested variables. trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java <http://review.hbase.org/r/229/#comment1301> This logic for mapping variable names to values has been repeated several times. Please move it to a dedicated method in the VariableSubstitution class. trunk/ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java <http://review.hbase.org/r/229/#comment1302> Please run checkstyle and fix any checkstyle errors that you have introduced. trunk/ql/src/test/queries/clientpositive/set_processor_namespaces.q <http://review.hbase.org/r/229/#comment1304> 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. - Carl 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 > >
