----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5507/#review8473 -----------------------------------------------------------
Looks good. I found a couple small things and made notes. Let me know what you think. Thanks. jdbc/src/java/org/apache/hadoop/hive/jdbc/Utils.java <https://reviews.apache.org/r/5507/#comment18147> Change the name to 'isEmbeddedMode'? jdbc/src/java/org/apache/hadoop/hive/jdbc/Utils.java <https://reviews.apache.org/r/5507/#comment18148> Space between method name and parameter list. jdbc/src/java/org/apache/hadoop/hive/jdbc/Utils.java <https://reviews.apache.org/r/5507/#comment18150> We may want to support syntax like this in the future: jdbc://<hostname>/<db_name>;x=1?hive.a=2 Where x=1 is a configuration parameter for the driver as opposed to something that should get set in the HiveConf. In order to be forward compatible with the future syntax we should make sure that dbName is the substring up to the first ';'. jdbc/src/java/org/apache/hadoop/hive/jdbc/Utils.java <https://reviews.apache.org/r/5507/#comment18149> The apache httpcomponents library has an URLEncodedUtils.parse() method that does this. I'd prefer that we use it here since it reduces our test burden and probably does escaping. http://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/client/utils/URLEncodedUtils.html jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java <https://reviews.apache.org/r/5507/#comment18151> Doesn't seem like the String arrays really add anything. May as well remove them. jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java <https://reviews.apache.org/r/5507/#comment18152> Might be a good idea to create a new connection handle for this test and set the configuration parameters for this URL only. - Carl Steinbach On June 22, 2012, 2:48 a.m., Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5507/ > ----------------------------------------------------------- > > (Updated June 22, 2012, 2:48 a.m.) > > > Review request for hive, Carl Steinbach and Carl Steinbach. > > > Description > ------- > > Support passing configuration and substitution variable as part of JDBC > connection string. > The new format of the URL is > jdbc:hive://<host>:<port>/dbName?hive_conf_list#hive_var_list , where the > optional conf and var lists are semicolon separated <key>=<val> pairs. As > before, if the host/port is not specified, it the driver runs an embedded > hive. > examples - > jdbc:hive://ubuntu:11000/db2?hive.cli.conf.printheader=true;hive.exec.mode.local.auto.inputbytes.max=9999#stab=salesTable;icol=customerID > jdbc:hive://?hive.cli.conf.printheader=true;hive.exec.mode.local.auto.inputbytes.max=9999#stab=salesTable;icol=customerID > > The patch include new routines to parse the URL. The conf values are added to > HiveConf when hive is running in embedded mode otherwise they are configured > using 'set' statement. The variable substitution is only used in case of > embedded mode. > > > This addresses bug HIVE-3166. > https://issues.apache.org/jira/browse/HIVE-3166 > > > Diffs > ----- > > jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveConnection.java 6618243 > jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDriver.java c61425f > jdbc/src/java/org/apache/hadoop/hive/jdbc/Utils.java 24d5882 > jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java f6a904f > > Diff: https://reviews.apache.org/r/5507/diff/ > > > Testing > ------- > > Ran JDBC tests. > Added new test case for the extended URL format. > > > Thanks, > > Prasad Mujumdar > >
