hadoop-hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Carl Steinbach" <c...@cloudera.com>
Subject Re: Review Request: HIVE-1096: Hive Variables
Date Tue, 29 Jun 2010 07:50:22 GMT

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


Mime
View raw message