You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Xuefu Zhang <xz...@cloudera.com> on 2013/06/02 15:45:39 UTC
Re: Review Request: HIVE-4568 Beeline needs to support resolving variables
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated June 2, 2013, 1:45 p.m.)
Review request for hive and Ashutosh Chauhan.
Description
-------
1. Added command variable substition
2. Added test case
This addresses bug HIVE-4568.
https://issues.apache.org/jira/browse/HIVE-4568
Diffs
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java aeb1e8b
beeline/src/java/org/apache/hive/beeline/TestBeeLineVarSubstitution.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/parse/VariableSubstitution.java f292944
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/#review25859
-----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BeeLine.java
<https://reviews.apache.org/r/11334/#comment50420>
thats fine
beeline/src/java/org/apache/hive/beeline/BeeLine.properties
<https://reviews.apache.org/r/11334/#comment50421>
It is more accurate to say that this is a hive specific variable, as beeline is still a generic tool.
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java
<https://reviews.apache.org/r/11334/#comment50422>
Yes, makes sense to call this hivevariables itself.
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java
<https://reviews.apache.org/r/11334/#comment50423>
This change has not been made in revised patch.
- Thejas Nair
On Aug. 24, 2013, 8:19 p.m., Xuefu Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11334/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2013, 8:19 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-4568
> https://issues.apache.org/jira/browse/HIVE-4568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> 1. Added command variable substition
> 2. Added test case
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
> beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
> beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
> beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
>
> Diff: https://reviews.apache.org/r/11334/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xuefu Zhang
>
>
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/#review26181
-----------------------------------------------------------
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java
<https://reviews.apache.org/r/11334/#comment51141>
This approach of setting the arguments going to be hard to read and maintain.
Can you do something like this ? -
replace the use of "private final String[] args" with a function "List<String> getBaseArgs(String jdbcUrl);"
Then add ("-f", scriptFileName) to the list it returns ?
Similarly add params to the list in testBeelineCommandLineHiveVariable ?
Everything else looks good.
- Thejas Nair
On Sept. 10, 2013, 9:45 p.m., Xuefu Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11334/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2013, 9:45 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-4568
> https://issues.apache.org/jira/browse/HIVE-4568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> 1. Added command variable substition
> 2. Added test case
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
> beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
> beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
> beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 4280449
>
> Diff: https://reviews.apache.org/r/11334/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xuefu Zhang
>
>
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/#review26198
-----------------------------------------------------------
Ship it!
Ship It!
- Thejas Nair
On Sept. 18, 2013, 12:41 a.m., Xuefu Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11334/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2013, 12:41 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-4568
> https://issues.apache.org/jira/browse/HIVE-4568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> 1. Added command variable substition
> 2. Added test case
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
> beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
> beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
> beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 4280449
>
> Diff: https://reviews.apache.org/r/11334/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xuefu Zhang
>
>
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated Sept. 18, 2013, 12:41 a.m.)
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-4568
https://issues.apache.org/jira/browse/HIVE-4568
Repository: hive-git
Description
-------
1. Added command variable substition
2. Added test case
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 4280449
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated Sept. 10, 2013, 9:45 p.m.)
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-4568
https://issues.apache.org/jira/browse/HIVE-4568
Repository: hive-git
Description
-------
1. Added command variable substition
2. Added test case
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 4280449
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated Sept. 4, 2013, 12:15 a.m.)
Review request for hive and Ashutosh Chauhan.
Changes
-------
I think I forgot to update the diff when patch HIVE-4568.3.patch was attached to the JIRA, which caused some confusion in the review comments. Now it's updated with latest review comments. Please review again to see if there is still anything missing. Thanks.
Bugs: HIVE-4568
https://issues.apache.org/jira/browse/HIVE-4568
Repository: hive-git
Description
-------
1. Added command variable substition
2. Added test case
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated Aug. 24, 2013, 8:19 p.m.)
Review request for hive and Ashutosh Chauhan.
Changes
-------
Diff is updated based on review feedback. The main changes are:
1. Separated code into a method dealing with appending hive variables to the connection url.
2. Usage info update.
Bugs: HIVE-4568
https://issues.apache.org/jira/browse/HIVE-4568
Repository: hive-git
Description
-------
1. Added command variable substition
2. Added test case
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/#review25500
-----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BeeLine.java
<https://reviews.apache.org/r/11334/#comment49947>
Okay. Will do
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java
<https://reviews.apache.org/r/11334/#comment49948>
HiveConf and HiveVar are two different sets of properties. The prefixes are:
HIVECONF_PREFIX = "hiveconf:";
HIVEVAR_PREFIX = "hivevar:";
Naming to hiveConfVars might cause confusion, as here we are dealing only hive variables.
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java
<https://reviews.apache.org/r/11334/#comment49949>
Same comments as above.
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java
<https://reviews.apache.org/r/11334/#comment49950>
Okay. Will do.
- Xuefu Zhang
On Aug. 21, 2013, 5:42 a.m., Xuefu Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11334/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2013, 5:42 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-4568
> https://issues.apache.org/jira/browse/HIVE-4568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> 1. Added command variable substition
> 2. Added test case
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
> beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
> beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
> beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
>
> Diff: https://reviews.apache.org/r/11334/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xuefu Zhang
>
>
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
> On Aug. 22, 2013, 8:09 a.m., Thejas Nair wrote:
> > beeline/src/java/org/apache/hive/beeline/BeeLine.java, line 509
> > <https://reviews.apache.org/r/11334/diff/3/?file=342835#file342835line509>
> >
> > I think having this in a function (parseHiveVariables(args) ?) would help in keeping the hive specific code separate
> >
When I attempted doing this, I found that parsing hive variables is an integral part of the for loop processing the commadline options. Separating it to another loop seems making the two loops more complicated because one loop will need to skip those args that are processed in the other. Thus, I kept this code unchanged. Let me know if you're okay with it.
- Xuefu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/#review25408
-----------------------------------------------------------
On Aug. 24, 2013, 8:19 p.m., Xuefu Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11334/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2013, 8:19 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-4568
> https://issues.apache.org/jira/browse/HIVE-4568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> 1. Added command variable substition
> 2. Added test case
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
> beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
> beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
> beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
>
> Diff: https://reviews.apache.org/r/11334/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xuefu Zhang
>
>
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/#review25408
-----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BeeLine.java
<https://reviews.apache.org/r/11334/#comment49816>
I think having this in a function (parseHiveVariables(args) ?) would help in keeping the hive specific code separate
beeline/src/java/org/apache/hive/beeline/BeeLine.properties
<https://reviews.apache.org/r/11334/#comment49815>
beeline is a generic jdbc tool and sqlline does not seem to having much active development. So it would be nice if it easy for people to figure out what parts are hive specific.
Can you put this at end of cmd-usage, maybe create a section where any future hive specific parameters can be added ?
I think we should make it clear that this gets set as hive config parameter for the connection.
What do you think of making this -hiveconf ? (i know it is inconsistent with rest of options). But since this is a hive specific option, we might as well make it consistent with other hive command line tools. That way we can make beeline easily with $HIVE_OPTS also.
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java
<https://reviews.apache.org/r/11334/#comment49817>
maybe call this hiveConfVars ?
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java
<https://reviews.apache.org/r/11334/#comment49818>
similar change to names here ?
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java
<https://reviews.apache.org/r/11334/#comment49819>
separating this into another function will help keep the hive specific code easy to trace.
- Thejas Nair
On Aug. 21, 2013, 5:42 a.m., Xuefu Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11334/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2013, 5:42 a.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-4568
> https://issues.apache.org/jira/browse/HIVE-4568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> 1. Added command variable substition
> 2. Added test case
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
> beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
> beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
> beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
>
> Diff: https://reviews.apache.org/r/11334/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xuefu Zhang
>
>
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated Aug. 21, 2013, 5:42 a.m.)
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-4568
https://issues.apache.org/jira/browse/HIVE-4568
Repository: hive-git
Description
-------
1. Added command variable substition
2. Added test case
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang
Re: Review Request 11334: HIVE-4568 Beeline needs to support resolving
variables
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11334/
-----------------------------------------------------------
(Updated Aug. 15, 2013, 5:33 a.m.)
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-4568
https://issues.apache.org/jira/browse/HIVE-4568
Repository: hive-git
Description
-------
1. Added command variable substition
2. Added test case
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/BeeLine.java 4c6eb9b
beeline/src/java/org/apache/hive/beeline/BeeLine.properties b6650cf
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 61bdeee
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java c70003d
beeline/src/test/org/apache/hive/beeline/src/test/TestBeeLineWithArgs.java 030f6b0
jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java 9fbc8ad
Diff: https://reviews.apache.org/r/11334/diff/
Testing
-------
Thanks,
Xuefu Zhang