----------------------------------------------------------- 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 On 2010-07-20 15:00:10, Carl Steinbach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/226/ > ----------------------------------------------------------- > > (Updated 2010-07-20 15:00:10) > > > Review request for Hive Developers. > > > Summary > ------- > > Submitted for review on behalf of Bennie Schut > > > This addresses bug HIVE-1126. > http://issues.apache.org/jira/browse/HIVE-1126 > > > Diffs > ----- > > trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveConnection.java 965834 > trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java > 965834 > trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java 965834 > trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSetMetaData.java > 965834 > trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcColumn.java > PRE-CREATION > trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcTable.java PRE-CREATION > trunk/jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java 965834 > > Diff: http://review.hbase.org/r/226/diff > > > Testing > ------- > > > Thanks, > > Carl > >
