Hive
  1. Hive
  2. HIVE-1126

Missing some Jdbc functionality like getTables getColumns and HiveResultSet.get* methods based on column name.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.7.0
    • Component/s: JDBC
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      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.

      1. HIVE-1126-7.patch
        193 kB
        Bennie Schut
      2. HIVE-1126-6.patch
        192 kB
        Bennie Schut
      3. HIVE-1126-5.patch
        192 kB
        Bennie Schut
      4. HIVE-1126-4.patch
        244 kB
        Bennie Schut
      5. HIVE-1126-3.patch
        244 kB
        Bennie Schut
      6. HIVE-1126-2.patch
        50 kB
        Bennie Schut
      7. HIVE-1126_patch(0.5.0_source).patch
        76 kB
        Sunil Kumar
      8. HIVE-1126-1.patch
        41 kB
        Bennie Schut
      9. HIVE-1126.patch
        39 kB
        Bennie Schut

        Issue Links

          Activity

          Hide
          John Sichi added a comment -

          For TABLE_TYPE in getTables, you can use the tableType attribute I recently added in HIVE-1068 (already committed). The mapping should be

          MANAGED_TABLE -> 'TABLE'
          VIRTUAL_VIEW -> 'VIEW'
          EXTERNAL_TABLE -> 'EXTERNAL TABLE'

          Show
          John Sichi added a comment - For TABLE_TYPE in getTables, you can use the tableType attribute I recently added in HIVE-1068 (already committed). The mapping should be MANAGED_TABLE -> 'TABLE' VIRTUAL_VIEW -> 'VIEW' EXTERNAL_TABLE -> 'EXTERNAL TABLE'
          Hide
          Bennie Schut added a comment -

          Ah nice.. I will add this and add the new patch..

          Show
          Bennie Schut added a comment - Ah nice.. I will add this and add the new patch..
          Hide
          Bennie Schut added a comment -

          client.get_tables only returns a list of tables names. The interface would need to be changed. Perhaps adding another method to would make sence?

          Show
          Bennie Schut added a comment - client.get_tables only returns a list of tables names. The interface would need to be changed. Perhaps adding another method to would make sence?
          Hide
          Bennie Schut added a comment -

          Ok for now I'll just call a client.get_table for each table which returns the table type. Tabletype however is a new feature so when pointing to an older hive server this field will be null so then I also let the jdbc driver return null for the TABLE_TYPE. I could return something like "TABLE" but I think this is ok for now.
          I'll make a new patch.

          Show
          Bennie Schut added a comment - Ok for now I'll just call a client.get_table for each table which returns the table type. Tabletype however is a new feature so when pointing to an older hive server this field will be null so then I also let the jdbc driver return null for the TABLE_TYPE. I could return something like "TABLE" but I think this is ok for now. I'll make a new patch.
          Hide
          Bennie Schut added a comment -

          new patch with table_type.

          Show
          Bennie Schut added a comment - new patch with table_type.
          Hide
          John Sichi added a comment -

          Hi Bennie,

          Thanks a lot for working on this. I have some initial review comments (and I may have some more after further review passes).

          1) Some new unit tests in TestJdbcDriver are needed for exercising this code.

          2) The pattern parameters for getTables and getColumns are not being honored. In the case of getTables, the tableNamePattern is ignored, whereas in the case of getColumns, tableNamePattern is assumed to be a specific table name, while columnNamePattern is ignored. All of these are insufficient in general, even if they are enough to get SQuirreL working. I don't think we should enable these API methods until we either implement them fully or make them throw exceptions when they detect cases they aren't capable of yet. For implementation, it's possible in some cases to translate the input JDBC-style pattern into the form of pattern Hive expects; in other cases, we may need to pull back more metadata than asked for and then do driver-side post-filtering.

          3) LIkewise for the types parameter to getTables. I've logged HIVE-1145 to get metastore API support for this, but in the meantime we can do it via driver-side post-filtering.

          4) We have comment metadata available for tables and columns, so we should return this for the relevant REMARKS columns in the result sets.

          5) Shouldn't the column names be ALL_CAPS (e.g. "TABLE_CAT" rather than "table_cat")?

          6) The JDBC spec requires the result sets to be returned in a definite order (specified in the JDBC API Javadoc for each method). We'll need a driver-side sort to accomplish this in some cases (e.g. for getTables, the rows are supposed to be ordered first by table type, and then by name).

          7) For getColumns, you've provided the ResultSetMetaData for the names/types of the returned columns; we need this consistent for getCatalogs/getTables as well.

          Show
          John Sichi added a comment - Hi Bennie, Thanks a lot for working on this. I have some initial review comments (and I may have some more after further review passes). 1) Some new unit tests in TestJdbcDriver are needed for exercising this code. 2) The pattern parameters for getTables and getColumns are not being honored. In the case of getTables, the tableNamePattern is ignored, whereas in the case of getColumns, tableNamePattern is assumed to be a specific table name, while columnNamePattern is ignored. All of these are insufficient in general, even if they are enough to get SQuirreL working. I don't think we should enable these API methods until we either implement them fully or make them throw exceptions when they detect cases they aren't capable of yet. For implementation, it's possible in some cases to translate the input JDBC-style pattern into the form of pattern Hive expects; in other cases, we may need to pull back more metadata than asked for and then do driver-side post-filtering. 3) LIkewise for the types parameter to getTables. I've logged HIVE-1145 to get metastore API support for this, but in the meantime we can do it via driver-side post-filtering. 4) We have comment metadata available for tables and columns, so we should return this for the relevant REMARKS columns in the result sets. 5) Shouldn't the column names be ALL_CAPS (e.g. "TABLE_CAT" rather than "table_cat")? 6) The JDBC spec requires the result sets to be returned in a definite order (specified in the JDBC API Javadoc for each method). We'll need a driver-side sort to accomplish this in some cases (e.g. for getTables, the rows are supposed to be ordered first by table type, and then by name). 7) For getColumns, you've provided the ResultSetMetaData for the names/types of the returned columns; we need this consistent for getCatalogs/getTables as well.
          Hide
          John Sichi added a comment -

          See also HIVE-1368.

          Show
          John Sichi added a comment - See also HIVE-1368 .
          Hide
          Sunil Kumar added a comment -

          Please use this patch(HIVE-1368.patch) this created from Hive _Trunk 0.5.0

          Show
          Sunil Kumar added a comment - Please use this patch( HIVE-1368 .patch) this created from Hive _Trunk 0.5.0
          Hide
          Sunil Kumar added a comment -

          Please ignore my above comment..
          Please use this patch( HIVE-1126_patch(0.5.0_source).patch) this created from Hive _Trunk 0.5.0

          Show
          Sunil Kumar added a comment - Please ignore my above comment.. Please use this patch( HIVE-1126 _patch(0.5.0_source).patch) this created from Hive _Trunk 0.5.0
          Hide
          Vinithra Varadharajan added a comment -

          Hi Bennie,
          Can I pick this JIRA up?
          Thanks.

          Show
          Vinithra Varadharajan added a comment - Hi Bennie, Can I pick this JIRA up? Thanks.
          Hide
          Bennie Schut added a comment -

          Hi Vinithra,

          I just started working on it again. I've had little time to work on this but in the end we also need this committed. But if I run into a problem I'll probably ask for some help.

          Thanks.

          Show
          Bennie Schut added a comment - Hi Vinithra, I just started working on it again. I've had little time to work on this but in the end we also need this committed. But if I run into a problem I'll probably ask for some help. Thanks.
          Hide
          Bennie Schut added a comment -

          John,

          I noticed schema support is geeing added (HIVE-675) but as far as I can tell there is no such thing as catalog's in hive right?
          From what I can tell:
          MySQL does not understand "schema", you have to use "catalog".
          Oracle does not understand "catalog", you have to use "schema".

          So it seems like it's more of a semantics thing. Since the other patch talks about schema's perhaps that's what we should also call it?

          fyi, I'm currently working on the points you mentioned.

          Bennie.

          Show
          Bennie Schut added a comment - John, I noticed schema support is geeing added ( HIVE-675 ) but as far as I can tell there is no such thing as catalog's in hive right? From what I can tell: MySQL does not understand "schema", you have to use "catalog". Oracle does not understand "catalog", you have to use "schema". So it seems like it's more of a semantics thing. Since the other patch talks about schema's perhaps that's what we should also call it? fyi, I'm currently working on the points you mentioned. Bennie.
          Hide
          Carl Steinbach added a comment -

          Hi Bennie,

          HIVE-675 will provide support in Hive for MySQL's concept of "databases", i.e. separate namespaces for tables.
          It will make it possible to do things like this:

          CREATE DATABASE d1;
          USE d1;
          CREATE TABLE t1(a INT);
          USE default;
          CREATE TABLE t1(b INT);
          

          HIVE-1010 addresses the task of adding support in Hive for an INFORMATION_SCHEMA.

          Show
          Carl Steinbach added a comment - Hi Bennie, HIVE-675 will provide support in Hive for MySQL's concept of "databases", i.e. separate namespaces for tables. It will make it possible to do things like this: CREATE DATABASE d1; USE d1; CREATE TABLE t1(a INT); USE default ; CREATE TABLE t1(b INT); HIVE-1010 addresses the task of adding support in Hive for an INFORMATION_SCHEMA.
          Hide
          Bennie Schut added a comment -

          Hi Carl,

          Thanks for the fast response. The jdbc driver has a concept of "catalog" and "schema" (this is jdbc terminology). You could for instance describe a table like this:
          Catalog.Schema.TableName
          But then it seems oracle implements the "getCatalogs" which allows you to do:
          Catalog.TableName
          Mysql supports the "getSchemas" which allows you to do this:
          Schema.TableName

          Now on mysql what they call a database = jdbc schema
          On oracle what they call a schema = jdbc catalog

          It seems nobody supports both and it seems to matter little which of the two you pick because the end result seems to allow you to do exactly the same thing.
          I have a personal preference in implementing getSchemas but if anyone can think of a reason to do it differently I would love to know.

          To me it seems like : schemas == databases == catalogs == namespaces

          Thanks,
          Bennie.

          Show
          Bennie Schut added a comment - Hi Carl, Thanks for the fast response. The jdbc driver has a concept of "catalog" and "schema" (this is jdbc terminology). You could for instance describe a table like this: Catalog.Schema.TableName But then it seems oracle implements the "getCatalogs" which allows you to do: Catalog.TableName Mysql supports the "getSchemas" which allows you to do this: Schema.TableName Now on mysql what they call a database = jdbc schema On oracle what they call a schema = jdbc catalog It seems nobody supports both and it seems to matter little which of the two you pick because the end result seems to allow you to do exactly the same thing. I have a personal preference in implementing getSchemas but if anyone can think of a reason to do it differently I would love to know. To me it seems like : schemas == databases == catalogs == namespaces Thanks, Bennie.
          Hide
          Carl Steinbach added a comment -

          Hi Benny,

          I think the convention in Hive is to mimic the behavior of MySQL whenever possible. Implementing "getSchemas" sounds like the right way to go.

          Show
          Carl Steinbach added a comment - Hi Benny, I think the convention in Hive is to mimic the behavior of MySQL whenever possible. Implementing "getSchemas" sounds like the right way to go.
          Hide
          Bennie Schut added a comment -

          In this new patch I addressed John's concern.

          1) Added a lot of new tests.
          2) Pattern matching should work now. It's client side but it works as per jdbc spec. (including tests)
          3) Agreed and done. (including tests)
          4) Remarks/comments are now also available on both columns and tables (including tests)
          5) Agreed and done.
          6) Agreed and done. (including tests)
          7) Agreed and done. (including tests)

          A lot has changed since the last patch. As a side effect it makes using the jdbc driver with SQuirreL a lot more stable/nicer to use.

          Bennie.

          Show
          Bennie Schut added a comment - In this new patch I addressed John's concern. 1) Added a lot of new tests. 2) Pattern matching should work now. It's client side but it works as per jdbc spec. (including tests) 3) Agreed and done. (including tests) 4) Remarks/comments are now also available on both columns and tables (including tests) 5) Agreed and done. 6) Agreed and done. (including tests) 7) Agreed and done. (including tests) A lot has changed since the last patch. As a side effect it makes using the jdbc driver with SQuirreL a lot more stable/nicer to use. Bennie.
          Hide
          Bennie Schut added a comment -

          Please let me know if we need more changes to get this committed.

          Thanks,
          Bennie.

          Show
          Bennie Schut added a comment - Please let me know if we need more changes to get this committed. Thanks, Bennie.
          Hide
          John Sichi added a comment -

          Hey, thanks a lot Bennie! I know a lot of people are looking forward to this one.

          I have a couple of cosmetic items which need to be fixed and then we can commit this:

          • JdbcColumn.java and JdbcTable.java: please add Apache file header to these two new files
          • HiveDatabaseMetaData.java: replace import java.util.* with expanded imports (Hive convention)

          Questions:

          • In HiveDatabaseMetaData.getTables, why did you need to make one of the blocks synchronized?
          • why does supportsNamedParameters return true?

          For the HiveResultSet getters (e.g. getLong), they are actually supposed to attempt a conversion (e.g. if the column is string, attempt convert to long). It's OK to just throw exception for now, but maybe the message should be "Unsupported conversion" instead of "Illegal conversion"?

          Regarding catalog/schema, I agree with Carl that we should try to mimic MySQL here.

          Show
          John Sichi added a comment - Hey, thanks a lot Bennie! I know a lot of people are looking forward to this one. I have a couple of cosmetic items which need to be fixed and then we can commit this: JdbcColumn.java and JdbcTable.java: please add Apache file header to these two new files HiveDatabaseMetaData.java: replace import java.util.* with expanded imports (Hive convention) Questions: In HiveDatabaseMetaData.getTables, why did you need to make one of the blocks synchronized? why does supportsNamedParameters return true? For the HiveResultSet getters (e.g. getLong), they are actually supposed to attempt a conversion (e.g. if the column is string, attempt convert to long). It's OK to just throw exception for now, but maybe the message should be "Unsupported conversion" instead of "Illegal conversion"? Regarding catalog/schema, I agree with Carl that we should try to mimic MySQL here.
          Hide
          Bennie Schut added a comment -

          Hi John,

          Thanks for the speedy comments.
          I'll make the fixes tomorrow morning.

          • Apache file header. I'll add them
          • Expanded imports. ok.
          • synchronizedon getTables. I forgot about that. When SQuirreL is rebuilding the tables tree a few times really fast (most likely concurrently) it seems to end up with the same table name multiple times in the list. I had a hunch this was related to the client not being completely thread safe (or the way I use it anyway). After making it synchronized I never saw this problem again. I'll also look into this a little more. On the "DatabaseMetaData getMetaData()" we could also decide to create a new client object which would probably also prevent this.
          • supportsNamedParameters sorry I confused it for something else. I'll revert that one.

          In most cases I ran a little test on the mysql jdbc driver to know what they returned and used that as a baseline.

          Show
          Bennie Schut added a comment - Hi John, Thanks for the speedy comments. I'll make the fixes tomorrow morning. Apache file header. I'll add them Expanded imports. ok. synchronizedon getTables. I forgot about that. When SQuirreL is rebuilding the tables tree a few times really fast (most likely concurrently) it seems to end up with the same table name multiple times in the list. I had a hunch this was related to the client not being completely thread safe (or the way I use it anyway). After making it synchronized I never saw this problem again. I'll also look into this a little more. On the "DatabaseMetaData getMetaData()" we could also decide to create a new client object which would probably also prevent this. supportsNamedParameters sorry I confused it for something else. I'll revert that one. In most cases I ran a little test on the mysql jdbc driver to know what they returned and used that as a baseline.
          Hide
          HBase Review Board added a comment -

          Message from: "Carl Steinbach" <carl@cloudera.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/226/
          -----------------------------------------------------------

          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

          Show
          HBase Review Board added a comment - Message from: "Carl Steinbach" <carl@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/226/ ----------------------------------------------------------- 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
          Hide
          HBase Review Board added a comment -

          Message from: "Carl Steinbach" <carl@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
          Show
          HBase Review Board added a comment - Message from: "Carl Steinbach" <carl@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
          Hide
          Bennie Schut added a comment -

          Ok some more on the synchronized client on getTables.
          This discussion on thrift show what I also noticed:
          http://mail-archives.apache.org/mod_mbox/incubator-thrift-user/200911.mbox/%3C6cf16f130911230449h1ab500e7y1c5ea787548f9da7@mail.gmail.com%3E

          So the way I see it is we have two options.
          1) we synchronize the client calls when we use them (like I already did). This should not be a problem since you wouldn't expect massive amounts of calls on this.
          2) when a call is made to Connection.getMetaData() we should create a new client connection and thus become thread safe.

          Option two has a bit more risk involving connection leaking. I noticed the close() on the HiveConnection is explicitly calling a "transport.close()" so I could keep track of all the connections we handout to also close during that close, however how do you cleanup a connection of a DatabaseMetaData object after it's used? If it's automatically garbage collected then I'm wondering why we have the explicit close now.

          Bennie.

          Show
          Bennie Schut added a comment - Ok some more on the synchronized client on getTables. This discussion on thrift show what I also noticed: http://mail-archives.apache.org/mod_mbox/incubator-thrift-user/200911.mbox/%3C6cf16f130911230449h1ab500e7y1c5ea787548f9da7@mail.gmail.com%3E So the way I see it is we have two options. 1) we synchronize the client calls when we use them (like I already did). This should not be a problem since you wouldn't expect massive amounts of calls on this. 2) when a call is made to Connection.getMetaData() we should create a new client connection and thus become thread safe. Option two has a bit more risk involving connection leaking. I noticed the close() on the HiveConnection is explicitly calling a "transport.close()" so I could keep track of all the connections we handout to also close during that close, however how do you cleanup a connection of a DatabaseMetaData object after it's used? If it's automatically garbage collected then I'm wondering why we have the explicit close now. Bennie.
          Hide
          Bennie Schut added a comment -

          Thinking some more about the sync. stuff. I guess if we would use synchronize we would also have to do it on other locations on the HiveDatabaseMetaData class.
          I'm done with the other changes so after we clear this up I can make a new patch.

          Show
          Bennie Schut added a comment - Thinking some more about the sync. stuff. I guess if we would use synchronize we would also have to do it on other locations on the HiveDatabaseMetaData class. I'm done with the other changes so after we clear this up I can make a new patch.
          Hide
          John Sichi added a comment -

          Regarding the synchronization: Squirrel really should not be calling onto multiple objects from the same JDBC connections from different threads (except in the special case of cancel). In general, JDBC drivers don't make a guarantee of thread safety within the same connection.

          If it is expecting this guarantee, then in general the synchronization would need to be added at a higher level (on the JDBC connection object itself, regardless of which object is receiving the API call) since over time we could be adding more client-side state.

          Creating a new client connection is not a good approach since we want the session state shared within a connection.

          I suggest we get the current work committed without any synchronization, and then open up a separate JIRA to investigate further.

          Regarding Carl's review comments: all of them look correct to me, so could you take care of them as part of the next rev of the patch?

          Show
          John Sichi added a comment - Regarding the synchronization: Squirrel really should not be calling onto multiple objects from the same JDBC connections from different threads (except in the special case of cancel). In general, JDBC drivers don't make a guarantee of thread safety within the same connection. If it is expecting this guarantee, then in general the synchronization would need to be added at a higher level (on the JDBC connection object itself, regardless of which object is receiving the API call) since over time we could be adding more client-side state. Creating a new client connection is not a good approach since we want the session state shared within a connection. I suggest we get the current work committed without any synchronization, and then open up a separate JIRA to investigate further. Regarding Carl's review comments: all of them look correct to me, so could you take care of them as part of the next rev of the patch?
          Hide
          Bennie Schut added a comment -

          John,

          I think the spec is pretty clear on this:
          http://download.oracle.com/docs/cd/E17476_01/javase/1.3/docs/guide/jdbc/spec/jdbc-spec.frame9.html
          We are supposed to make it thread safe. Now they explain that the most logical use case for multi threading on a single connection would be the cancel call. What squirrel is doing might be unexpected but it is allowed per spec. A separate JIRA would make sense since this isn't the only spot I guess.
          I'll remove the sync for now.

          I've changed all things Carl said except for the copying the input lists. These lists are explicitly created to be used on this resultset only and can't possibly be modified by anything. I think I should have made them final like this:

          public HiveMetaDataResultSet(final List<String> columnNames
          , final List<String> columnTypes

          Which would make this clear. However if anyone feels strongly about having this copied by value then I'll change that to.

          You can expect a new patch in a few minutes.

          Bennie.

          Show
          Bennie Schut added a comment - John, I think the spec is pretty clear on this: http://download.oracle.com/docs/cd/E17476_01/javase/1.3/docs/guide/jdbc/spec/jdbc-spec.frame9.html We are supposed to make it thread safe. Now they explain that the most logical use case for multi threading on a single connection would be the cancel call. What squirrel is doing might be unexpected but it is allowed per spec. A separate JIRA would make sense since this isn't the only spot I guess. I'll remove the sync for now. I've changed all things Carl said except for the copying the input lists. These lists are explicitly created to be used on this resultset only and can't possibly be modified by anything. I think I should have made them final like this: public HiveMetaDataResultSet(final List<String> columnNames , final List<String> columnTypes Which would make this clear. However if anyone feels strongly about having this copied by value then I'll change that to. You can expect a new patch in a few minutes. Bennie.
          Hide
          Bennie Schut added a comment -

          New patch with changes requested by Carl and John.

          Show
          Bennie Schut added a comment - New patch with changes requested by Carl and John.
          Hide
          Carl Steinbach added a comment -

          I've changed all things Carl said except for the copying the input lists. These lists are explicitly created to be used on this resultset only and can't possibly be modified by anything. I think I should have made them final like this

          In Java a final variable can only be assigned to once. Making a variable final does not make it immutable, and making the constructor parameters columnNames and columnTypes final won't prevent the caller from later mutating these lists and affecting the internal state of the ResultSet object. I think it's prudent to make this change now since it's hard to anticipate how this class will be used in the future.

          Show
          Carl Steinbach added a comment - I've changed all things Carl said except for the copying the input lists. These lists are explicitly created to be used on this resultset only and can't possibly be modified by anything. I think I should have made them final like this In Java a final variable can only be assigned to once. Making a variable final does not make it immutable, and making the constructor parameters columnNames and columnTypes final won't prevent the caller from later mutating these lists and affecting the internal state of the ResultSet object. I think it's prudent to make this change now since it's hard to anticipate how this class will be used in the future.
          Hide
          Bennie Schut added a comment -

          I'll make the change now.

          Show
          Bennie Schut added a comment - I'll make the change now.
          Hide
          Bennie Schut added a comment -

          Now using copy by value.

          Show
          Bennie Schut added a comment - Now using copy by value.
          Hide
          John Sichi added a comment -

          Thanks for the multi-threading reference; I wasn't aware of that. OK, we should try to address that comprehensively in HIVE-1482.

          Show
          John Sichi added a comment - Thanks for the multi-threading reference; I wasn't aware of that. OK, we should try to address that comprehensively in HIVE-1482 .
          Hide
          John Sichi added a comment -

          Bennie, I'm having trouble applying HIVE-1126-4.patch. Did you base it against trunk?

          can't find file to patch at input line 6924
          Perhaps you used the wrong -p or --strip option?
          The text leading up to this was:
          --------------------------

          Index: jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java
          ===================================================================
          — jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java (revision 965833)
          +++ jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java (working copy)
          --------------------------
          File to patch:
          Show
          John Sichi added a comment - Bennie, I'm having trouble applying HIVE-1126 -4.patch. Did you base it against trunk? can't find file to patch at input line 6924 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- Index: jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java =================================================================== — jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java (revision 965833) +++ jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java (working copy) -------------------------- File to patch:
          Hide
          Bennie Schut added a comment -

          Somehow the previous patch was incomplete. not sure why but this was it's state on svn:
          A + src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java
          I removed it and added it again and now the patch is complete again:

          patch -p0 --dry-run < HIVE-1126-5.patch
          patching file jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveCallableStatement.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveBaseResultSet.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HivePreparedStatement.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSetMetaData.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveMetaDataResultSet.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcTable.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveConnection.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcColumn.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveStatement.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java
          patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java

          Show
          Bennie Schut added a comment - Somehow the previous patch was incomplete. not sure why but this was it's state on svn: A + src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java I removed it and added it again and now the patch is complete again: patch -p0 --dry-run < HIVE-1126 -5.patch patching file jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveCallableStatement.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveBaseResultSet.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HivePreparedStatement.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSetMetaData.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveMetaDataResultSet.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcTable.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveConnection.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcColumn.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveStatement.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java patching file jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java
          Hide
          Amr Awadallah added a comment -

          I am out of office on vacation and will be slower than usual in
          responding to emails. If this is urgent then please call my cell phone
          (or send an sms), otherwise I will reply to your email when I get
          back.

          Thanks for your patience,

          – amr

          Show
          Amr Awadallah added a comment - I am out of office on vacation and will be slower than usual in responding to emails. If this is urgent then please call my cell phone (or send an sms), otherwise I will reply to your email when I get back. Thanks for your patience, – amr
          Hide
          John Sichi added a comment -

          TestJdbcDriver is not passing: one failure and one error.

          <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetTables" time="0.977">
          <failure message="expected:<src> but was:<testhivedriverpartitionedtable>" type="junit.framework.AssertionFailedError">junit.framework.AssertionFailedError: expected:<src> but was\
          :<testhivedriverpartitionedtable>
          at junit.framework.Assert.fail(Assert.java:47)
          at junit.framework.Assert.failNotEquals(Assert.java:282)
          at junit.framework.Assert.assertEquals(Assert.java:64)
          at junit.framework.Assert.assertEquals(Assert.java:71)
          at org.apache.hadoop.hive.jdbc.TestJdbcDriver.testMetaDataGetTables(TestJdbcDriver.java:334)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at junit.framework.TestCase.runTest(TestCase.java:154)
          at junit.framework.TestCase.runBare(TestCase.java:127)
          at junit.framework.TestResult$1.protect(TestResult.java:106)
          at junit.framework.TestResult.runProtected(TestResult.java:124)
          at junit.framework.TestResult.run(TestResult.java:109)
          at junit.framework.TestCase.run(TestCase.java:118)
          at junit.framework.TestSuite.runTest(TestSuite.java:208)
          at junit.framework.TestSuite.run(TestSuite.java:203)
          at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:422)
          at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:931)
          at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:785)
          </failure>
          </testcase>
          <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetCatalogs" time="0.819" />
          <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetTableTypes" time="0.753" />
          <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetColumns" time="0.967">
          <error message="Unrecognized column type: array<int>" type="java.sql.SQLException">java.sql.SQLException: Unrecognized column type: array<int>
          at org.apache.hadoop.hive.jdbc.HiveResultSetMetaData.hiveTypeToSqlType(HiveResultSetMetaData.java:119)
          at org.apache.hadoop.hive.jdbc.JdbcColumn.getSqlType(JdbcColumn.java:61)
          at org.apache.hadoop.hive.jdbc.HiveDatabaseMetaData$2.next(HiveDatabaseMetaData.java:198)
          at org.apache.hadoop.hive.jdbc.TestJdbcDriver.testMetaDataGetColumns(TestJdbcDriver.java:411)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at junit.framework.TestCase.runTest(TestCase.java:154)
          at junit.framework.TestCase.runBare(TestCase.java:127)

          Show
          John Sichi added a comment - TestJdbcDriver is not passing: one failure and one error. <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetTables" time="0.977"> <failure message="expected:<src> but was:<testhivedriverpartitionedtable>" type="junit.framework.AssertionFailedError">junit.framework.AssertionFailedError: expected:<src> but was\ :<testhivedriverpartitionedtable> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:282) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:71) at org.apache.hadoop.hive.jdbc.TestJdbcDriver.testMetaDataGetTables(TestJdbcDriver.java:334) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:154) at junit.framework.TestCase.runBare(TestCase.java:127) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:118) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:422) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:931) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:785) </failure> </testcase> <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetCatalogs" time="0.819" /> <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetTableTypes" time="0.753" /> <testcase classname="org.apache.hadoop.hive.jdbc.TestJdbcDriver" name="testMetaDataGetColumns" time="0.967"> <error message="Unrecognized column type: array<int>" type="java.sql.SQLException">java.sql.SQLException: Unrecognized column type: array<int> at org.apache.hadoop.hive.jdbc.HiveResultSetMetaData.hiveTypeToSqlType(HiveResultSetMetaData.java:119) at org.apache.hadoop.hive.jdbc.JdbcColumn.getSqlType(JdbcColumn.java:61) at org.apache.hadoop.hive.jdbc.HiveDatabaseMetaData$2.next(HiveDatabaseMetaData.java:198) at org.apache.hadoop.hive.jdbc.TestJdbcDriver.testMetaDataGetColumns(TestJdbcDriver.java:411) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:154) at junit.framework.TestCase.runBare(TestCase.java:127)
          Hide
          Bennie Schut added a comment -

          Hi John,

          Thanks for looking at this. I did this:

          svn co http://svn.apache.org/repos/asf/hadoop/hive/trunk hive_test/
          patch -p0 < HIVE-1126-5.patch
          ant test -Dtestcase=TestJdbcDriver -Doverwrite=true

          BUILD SUCCESSFUL
          Total time: 1 minute 37 seconds

          However the error is correct. I don't think we support array on jdbc. Non of the tables on TestJdbcDriver seem to use the array type so perhaps there is another ticket related to arrays which is colliding with this one?

          Show
          Bennie Schut added a comment - Hi John, Thanks for looking at this. I did this: svn co http://svn.apache.org/repos/asf/hadoop/hive/trunk hive_test/ patch -p0 < HIVE-1126 -5.patch ant test -Dtestcase=TestJdbcDriver -Doverwrite=true BUILD SUCCESSFUL Total time: 1 minute 37 seconds However the error is correct. I don't think we support array on jdbc. Non of the tables on TestJdbcDriver seem to use the array type so perhaps there is another ticket related to arrays which is colliding with this one?
          Hide
          Bennie Schut added a comment -

          I just noticed you only receive this error when you run a full test. I'll look into it a bit more.

          Show
          Bennie Schut added a comment - I just noticed you only receive this error when you run a full test. I'll look into it a bit more.
          Hide
          Bennie Schut added a comment -

          As I expected it is colliding with other tests I think I'll just remove the check for "all" tables since it is returning a few more tables from other tests.
          It's only visible after running the full tests so it will take a few hours before I can upload the new patch.

          Show
          Bennie Schut added a comment - As I expected it is colliding with other tests I think I'll just remove the check for "all" tables since it is returning a few more tables from other tests. It's only visible after running the full tests so it will take a few hours before I can upload the new patch.
          Hide
          Bennie Schut added a comment -

          Sorry about the previously failing tests. I now take into account some of the other tests might leave some tables laying around.

          I now did a full test run:
          BUILD SUCCESSFUL
          Total tiame: 77 minutes 38 seconds

          Show
          Bennie Schut added a comment - Sorry about the previously failing tests. I now take into account some of the other tests might leave some tables laying around. I now did a full test run: BUILD SUCCESSFUL Total tiame: 77 minutes 38 seconds
          Hide
          HBase Review Board added a comment -

          Message from: "John Sichi" <jsichi@facebook.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/385/
          -----------------------------------------------------------

          Review request for Hive Developers.

          Summary
          -------

          Review for HIVE-1126 patch 6 by jvs.

          This addresses bug HIVE-1126.
          http://issues.apache.org/jira/browse/HIVE-1126

          Diffs


          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveBaseResultSet.java PRE-CREATION
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveCallableStatement.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveConnection.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveMetaDataResultSet.java PRE-CREATION
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HivePreparedStatement.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java PRE-CREATION
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSetMetaData.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveStatement.java 978817
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcColumn.java PRE-CREATION
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcTable.java PRE-CREATION
          http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java 978817

          Diff: http://review.hbase.org/r/385/diff

          Testing
          -------

          Thanks,

          John

          Show
          HBase Review Board added a comment - Message from: "John Sichi" <jsichi@facebook.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/385/ ----------------------------------------------------------- Review request for Hive Developers. Summary ------- Review for HIVE-1126 patch 6 by jvs. This addresses bug HIVE-1126 . http://issues.apache.org/jira/browse/HIVE-1126 Diffs http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveBaseResultSet.java PRE-CREATION http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveCallableStatement.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveConnection.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveDatabaseMetaData.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveMetaDataResultSet.java PRE-CREATION http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HivePreparedStatement.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveQueryResultSet.java PRE-CREATION http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSet.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSetMetaData.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveStatement.java 978817 http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcColumn.java PRE-CREATION http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/java/org/apache/hadoop/hive/jdbc/JdbcTable.java PRE-CREATION http://svn.apache.org/repos/asf/hadoop/hive/trunk/jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java 978817 Diff: http://review.hbase.org/r/385/diff Testing ------- Thanks, John
          Hide
          HBase Review Board added a comment -

          Message from: "Carl Steinbach" <carl@cloudera.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.hbase.org/r/385/#review486
          -----------------------------------------------------------

          Ship it!

          +1 Looks good to me.

          • Carl
          Show
          HBase Review Board added a comment - Message from: "Carl Steinbach" <carl@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/385/#review486 ----------------------------------------------------------- Ship it! +1 Looks good to me. Carl
          Hide
          John Sichi added a comment -

          +1. Will commit when tests pass.

          Show
          John Sichi added a comment - +1. Will commit when tests pass.
          Hide
          John Sichi added a comment -

          I am still getting a failure in TestJdbcDriver when run as part of the full ant test (below).

          Also, note that the failure message is confusing because when you use Assert.assertEquals(x, y), x is supposed to be the expected (correct) value and y is supposed to be the actual value produced by the test, but the code has them reversed here.

             <failure message="expected:&lt;testhivedrivertable&gt; but was:&lt;testhive\
          jdbcdriverpartitionedtable&gt;" type="junit.framework.AssertionFailedError">jun\
          it.framework.AssertionFailedError: expected:&lt;testhivedrivertable&gt; but was\
          :&lt;testhivejdbcdriverpartitionedtable&gt;                                     
                  at junit.framework.Assert.fail(Assert.java:47)                          
                  at junit.framework.Assert.failNotEquals(Assert.java:282)                
                  at junit.framework.Assert.assertEquals(Assert.java:64)                  
                  at junit.framework.Assert.assertEquals(Assert.java:71)                  
                  at org.apache.hadoop.hive.jdbc.TestJdbcDriver.testMetaDataGetTables(Tes\
          tJdbcDriver.java:331)                                                           
                  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)          
                  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl\
          .java:39)                                                                       
                  at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAcce\
          ssorImpl.java:25)                                                               
                  at java.lang.reflect.Method.invoke(Method.java:597)                     
                  at junit.framework.TestCase.runTest(TestCase.java:154)                  
                  at junit.framework.TestCase.runBare(TestCase.java:127)                  
                  at junit.framework.TestResult$1.protect(TestResult.java:106)            
                  at junit.framework.TestResult.runProtected(TestResult.java:124)         
                  at junit.framework.TestResult.run(TestResult.java:109)                  
                  at junit.framework.TestCase.run(TestCase.java:118)                      
                  at junit.framework.TestSuite.runTest(TestSuite.java:208)                
                  at junit.framework.TestSuite.run(TestSuite.java:203)                    
                  at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUn\
          itTestRunner.java:422)                                                          
                  at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(\
          JUnitTestRunner.java:931)                                                       
                  at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JU\
          nitTestRunner.java:785)                                                         
          </failure>                                                                      
          
          Show
          John Sichi added a comment - I am still getting a failure in TestJdbcDriver when run as part of the full ant test (below). Also, note that the failure message is confusing because when you use Assert.assertEquals(x, y), x is supposed to be the expected (correct) value and y is supposed to be the actual value produced by the test, but the code has them reversed here. <failure message="expected:&lt;testhivedrivertable&gt; but was:&lt;testhive\ jdbcdriverpartitionedtable&gt;" type="junit.framework.AssertionFailedError">jun\ it.framework.AssertionFailedError: expected:&lt;testhivedrivertable&gt; but was\ :&lt;testhivejdbcdriverpartitionedtable&gt; at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:282) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:71) at org.apache.hadoop.hive.jdbc.TestJdbcDriver.testMetaDataGetTables(Tes\ tJdbcDriver.java:331) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl\ .java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAcce\ ssorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:154) at junit.framework.TestCase.runBare(TestCase.java:127) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:118) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUn\ itTestRunner.java:422) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(\ JUnitTestRunner.java:931) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JU\ nitTestRunner.java:785) </failure>
          Hide
          John Sichi added a comment -

          (the backslash line continuations are because I copied out of an Emacs buffer)

          Show
          John Sichi added a comment - (the backslash line continuations are because I copied out of an Emacs buffer)
          Hide
          Bennie Schut added a comment -

          Sorry I seem to have uploaded the wrong patch. Since I have to upload it again I'll also fix the confusing error messages.

          Show
          Bennie Schut added a comment - Sorry I seem to have uploaded the wrong patch. Since I have to upload it again I'll also fix the confusing error messages.
          Hide
          Bennie Schut added a comment -

          I keep getting errors on my test run on the test: testCliDriver_loadpart_err which seem unrelated to my changes.

          Show
          Bennie Schut added a comment - I keep getting errors on my test run on the test: testCliDriver_loadpart_err which seem unrelated to my changes.
          Hide
          Amr Awadallah added a comment -

          I am out of office on vacation and will be slower than usual in
          responding to emails. If this is urgent then please call my cell phone
          (or send an sms), otherwise I will reply to your email when I get
          back.

          Thanks for your patience,

          – amr

          Show
          Amr Awadallah added a comment - I am out of office on vacation and will be slower than usual in responding to emails. If this is urgent then please call my cell phone (or send an sms), otherwise I will reply to your email when I get back. Thanks for your patience, – amr
          Hide
          John Sichi added a comment -

          @Bennie: yeah, it is flaking for me too. It has been flaky forever but seems to have gotten worse for me recently. I've logged HIVE-1491 to disable it, but until we get that done, you can just delete loadpart_err.q and loadpart_err.q.out before running ant package test.

          Show
          John Sichi added a comment - @Bennie: yeah, it is flaking for me too. It has been flaky forever but seems to have gotten worse for me recently. I've logged HIVE-1491 to disable it, but until we get that done, you can just delete loadpart_err.q and loadpart_err.q.out before running ant package test.
          Hide
          Bennie Schut added a comment -

          New patch with fixed test. Also switched the actual/expected values so they are now correct plus added some messages which should make any failing test more clear.

          Show
          Bennie Schut added a comment - New patch with fixed test. Also switched the actual/expected values so they are now correct plus added some messages which should make any failing test more clear.
          Hide
          John Sichi added a comment -

          Committed. Thanks Bennie!

          Show
          John Sichi added a comment - Committed. Thanks Bennie!

            People

            • Assignee:
              Bennie Schut
              Reporter:
              Bennie Schut
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development