[
https://issues.apache.org/jira/browse/HIVE-1126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12890506#action_12890506
]
HBase Review Board commented on HIVE-1126:
------------------------------------------
Message from: "Carl Steinbach" <c...@cloudera.com>
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/226/#review433
-----------------------------------------------------------
Hi Benny,
I have some suggestions. Sorry I didn't make these earlier.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1796>
String constants like this should probably be defined as static final
strings at the top of the file. Better yet would be to add a HiveConstants
class to common and define them all there so that other parts of the code can
reference them.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1798>
Please use an ArrayList by default.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1801>
Please create a copy of the input list.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1797>
Please run 'ant checkstyle' and fix any errors/warnings that you have
introduced.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1802>
The initial size of this list is set to 9 but you always add 19 elements?
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1806>
I'm concerned that extending and selectively overriding components of
HiveResultSet is the wrong thing to do here. HiveResultSet contains a lot of
stuff that you don't want in this case (such as a potential reference to the
thrift client), and there's always the chance that someone add new code to
HiveResultSet that would then need to override here.
I recommend creating a HiveBaseResultSet class that has everything
disabled, and then extending that with a HiveMetadataResultSet (that you would
use here) and a HiveQueryResultSet that would take the place of the current
HiveResultSet.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1803>
Why is the initial size set to 5?
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1804>
There's no need to use explicit indexes when adding elements to an
ArrayList. In this case it also opens the door to someone introducing a bug in
the future if the modify the code and forget to update the indexing.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
<http://review.hbase.org/r/226/#comment1807>
Please replace with HiveMetadataResultSet
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java
<http://review.hbase.org/r/226/#comment1808>
Please change the name of this class to HiveQueryResultSet and have it
extend HiveBaseResultSet.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java
<http://review.hbase.org/r/226/#comment1799>
Please copy the input lists, e.g:
this.columnNames = new ArrayList<String>(columnNames);
this.columnTypes = new ArrayList<String>(columnTypes);
I think there are other places where this needs to be done as well.
Also, this constructor should not be here since it leaves this.client
uninitialized and opens the possibility of an NPE if someone forgets to
override next().
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java
<http://review.hbase.org/r/226/#comment1809>
This method should be private.
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java
<http://review.hbase.org/r/226/#comment1800>
Checkstyle error: you need {}s
trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java
<http://review.hbase.org/r/226/#comment1810>
This throws an NPE if I call close() and then next(), or if I call one of
the constructors that does not set client and then call next(). In the first
case I think the proper behavior is to throw a SQLException. I think the second
case should be made impossible by making the changes I mentioned above (i.e.
split this into abstract HiveBaseResultSet, HiveMetadataResultSet, and
HiveQueryResultSet).
- Carl
> Missing some Jdbc functionality like getTables getColumns and
> HiveResultSet.get* methods based on column name.
> --------------------------------------------------------------------------------------------------------------
>
> Key: HIVE-1126
> URL: https://issues.apache.org/jira/browse/HIVE-1126
> Project: Hadoop Hive
> Issue Type: Improvement
> Components: Clients
> Affects Versions: 0.7.0
> Reporter: Bennie Schut
> Assignee: Bennie Schut
> Priority: Minor
> Fix For: 0.7.0
>
> Attachments: HIVE-1126-1.patch, HIVE-1126-2.patch, HIVE-1126.patch,
> HIVE-1126_patch(0.5.0_source).patch
>
>
> I've been using the hive jdbc driver more and more and was missing some
> functionality which I added
> HiveDatabaseMetaData.getTables
> Using "show tables" to get the info from hive.
> HiveDatabaseMetaData.getColumns
> Using "describe tablename" to get the columns.
> This makes using something like SQuirreL a lot nicer since you have the list
> of tables and just click on the content tab to see what's in the table.
> I also implemented
> HiveResultSet.getObject(String columnName) so you call most get* methods
> based on the column name.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
