[
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" <c...@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.
