> On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > I think it's important to get local mode working and also put a Java > > wrapper in place before committing this.
ok. I will make changes and update the patch. > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > LICENSE, line 421 > > <https://reviews.apache.org/r/5381/diff/2/?file=111880#file111880line421> > > > > Please replace with the license text from CVS. I am still not fully convinced that copying the CVS license is the right way. Here are following things to consider - 1) The source distributions (even 1.0.2) still includes the old license file which contains GNU license. 2) In fact this license change from GNU to BSD was done in 1.0.2 release, so I am not sure if that makes 1.0 a GNU license release. Just to be safe, I put a added a workaround in the ivy script to use 1.0.2. Though on a related note, I am not sure if its possible to change license from GNU to non-GNU on the same codebase ... 3) The binary distributions are not including the license text in CVS repository in any format, also its not there on the download site. Hence I don't know if this qualifies to be a license under which Hive is using/redistributing sqlline. > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > bin/ext/sqlline.sh, line 16 > > <https://reviews.apache.org/r/5381/diff/2/?file=111882#file111882line16> > > > > Missing "[db]". Also, this is what sqlline_help() should print. Done > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > bin/ext/sqlline.sh, line 17 > > <https://reviews.apache.org/r/5381/diff/2/?file=111882#file111882line17> > > > > Would you mind changing the name to "beeline"? That was the name John > > used in his original patch and I think it's kind of cute. Done > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > bin/ext/sqlline.sh, line 27 > > <https://reviews.apache.org/r/5381/diff/2/?file=111882#file111882line27> > > > > Please use getopts > > (http://rsalveti.wordpress.com/2007/04/03/bash-parsing-arguments-with-getopts/) > > or delegate command line option handling to a Java wrapper. The latter > > option is preferred since it's more portable and is arguably less fragile. yes, will pass it to the java warpper and include in the updated patch. > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > bin/ext/sqlline.sh, line 32 > > <https://reviews.apache.org/r/5381/diff/2/?file=111882#file111882line32> > > > > Where is execSQLLine defined? removed > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > bin/ext/sqlline.sh, line 37 > > <https://reviews.apache.org/r/5381/diff/2/?file=111882#file111882line37> > > > > The help options that get dumped here are inaccessible from the command > > line. changed to print the hive options > On June 18, 2012, 7:36 p.m., Carl Steinbach wrote: > > cli/ivy.xml, line 40 > > <https://reviews.apache.org/r/5381/diff/2/?file=111883#file111883line40> > > > > I think it probably makes more sense for this to be a dependency on the > > hive-jdbc module. Otherwise we also need to add a hive-jdbc dependency to > > hive-cli which is something I would rather avoid. > > > > On a related note, the BeeLine wrapper and command processor also > > belongs in the hive-jdbc module. hmm .. ok. will do that. - Prasad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5381/#review8346 ----------------------------------------------------------- On June 18, 2012, 6:19 p.m., Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5381/ > ----------------------------------------------------------- > > (Updated June 18, 2012, 6:19 p.m.) > > > Review request for hive and Carl Steinbach. > > > Description > ------- > > The patch is to include command line SQL editor SQLLine into Hive > distribution. The tool can be invoked using 'hive --service sqlline <host> > [port]'. It requires the HiveServer running on the given host/port. > > The ivy dependencies are updated to include sqlline. The hive scripts are > updated to executing SQLLine with the required connection URL and other > command line options. The LICENSE and NOTICE files are updated to include > SQLLine information. > > > Diffs > ----- > > LICENSE 05085da > NOTICE 871fdde > bin/ext/sqlline.sh PRE-CREATION > cli/ivy.xml ab949b1 > ivy/ivysettings.xml fb6f4b8 > ivy/libraries.properties 8461da1 > > Diff: https://reviews.apache.org/r/5381/diff/ > > > Testing > ------- > > > Thanks, > > Prasad Mujumdar > >
