hadoop-hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "HBase Review Board (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HIVE-1096) Hive Variables
Date Tue, 29 Jun 2010 07:56:54 GMT

    [ https://issues.apache.org/jira/browse/HIVE-1096?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12883453#action_12883453
] 

HBase Review Board commented on HIVE-1096:
------------------------------------------

Message from: "Carl Steinbach" <carl@cloudera.com>

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





> Hive Variables
> --------------
>
>                 Key: HIVE-1096
>                 URL: https://issues.apache.org/jira/browse/HIVE-1096
>             Project: Hadoop Hive
>          Issue Type: New Feature
>          Components: Query Processor
>            Reporter: Edward Capriolo
>            Assignee: Edward Capriolo
>             Fix For: 0.6.0, 0.7.0
>
>         Attachments: 1096-9.diff, hive-1096-10-patch.txt, hive-1096-11-patch.txt, hive-1096-2.diff,
hive-1096-7.diff, hive-1096-8.diff, hive-1096.diff
>
>
> From mailing list:
> --Amazon Elastic MapReduce version of Hive seems to have a nice feature called "Variables."
Basically you can define a variable via command-line while invoking hive with -d DT=2009-12-09
and then refer to the variable via ${DT} within the hive queries. This could be extremely
useful. I can't seem to find this feature even on trunk. Is this feature currently anywhere
in the roadmap?--
> This could be implemented in many places.
> A simple place to put this is 
> in Driver.compile or Driver.run we can do string substitutions at that level, and further
downstream need not be effected. 
> There could be some benefits to doing this further downstream, parser,plan. but based
on the simple needs we may not need to overthink this.
> I will get started on implementing in compile unless someone wants to discuss this more.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message