----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54711/#review159187 -----------------------------------------------------------
jdbc-handler/pom.xml (lines 82 - 86) <https://reviews.apache.org/r/54711/#comment230134> Could you please clarify why jdbc connector depends on hbase? jdbc-handler/src/java/org/apache/hive/storagehandler/JDBCStorageHandler.java (line 27) <https://reviews.apache.org/r/54711/#comment230136> Style: extra line is not required. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java (line 10) <https://reviews.apache.org/r/54711/#comment230137> It is better to make this class *final*. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java (line 11) <https://reviews.apache.org/r/54711/#comment230138> Please add a private constructor to avoid accidental creation of an instance of this utility class. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java (line 21) <https://reviews.apache.org/r/54711/#comment230142> Should we take into account version of the DB? jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java (line 24) <https://reviews.apache.org/r/54711/#comment230139> It is better to use enumeration instead of plain string. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java (line 33) <https://reviews.apache.org/r/54711/#comment230140> I think it will be more user friendly and informative, to say "XXXX database is not supported" If you have an enumeration of supported DBs, than message might be "Currently supported databases: {}, but found {}". jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java (line 38) <https://reviews.apache.org/r/54711/#comment230141> Extra line is not required. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 19) <https://reviews.apache.org/r/54711/#comment230220> Returns... jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 26) <https://reviews.apache.org/r/54711/#comment230218> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 28) <https://reviews.apache.org/r/54711/#comment230232> Is it necessary to pass DBConfig every time? It could be used once when the instance is created. I think a factory will work well here. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 31) <https://reviews.apache.org/r/54711/#comment230219> Returns... jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 34) <https://reviews.apache.org/r/54711/#comment230222> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 39) <https://reviews.apache.org/r/54711/#comment230223> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 44) <https://reviews.apache.org/r/54711/#comment230221> Provides ... jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 56) <https://reviews.apache.org/r/54711/#comment230224> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 68) <https://reviews.apache.org/r/54711/#comment230225> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 73) <https://reviews.apache.org/r/54711/#comment230226> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 78) <https://reviews.apache.org/r/54711/#comment230228> Provides... in the table. jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java (line 85) <https://reviews.apache.org/r/54711/#comment230227> @return is missing jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleJDBCDBWritable.java (line 31) <https://reviews.apache.org/r/54711/#comment230233> Should HiveJDBCTypeBridgeUtil be used to create an instance of the bridge? jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleJDBCDBWritable.java (line 37) <https://reviews.apache.org/r/54711/#comment230235> It is better to do for (Map.Entry<String, Class<?>> entry : columnTypeMapping.entrySet()) { ... } jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleTypeBridge.java (line 8) <https://reviews.apache.org/r/54711/#comment230236> Style: It seems like in Hive code base annotations are on a separate line. Please make sure your files pass checkstyle. jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleVendorBridge.java (line 25) <https://reviews.apache.org/r/54711/#comment230238> It is better to use try-catch with resource: http://stackoverflow.com/questions/9260159/java-7-automatic-resource-management-jdbc-try-with-resources-statement jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleVendorBridge.java (line 38) <https://reviews.apache.org/r/54711/#comment230239> Allocated resources are not released. jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleVendorBridge.java (line 74) <https://reviews.apache.org/r/54711/#comment230240> return String.format("SELECT \* FROM %s WHERE ROWNUM = 0", tableName); The name should be escaped. For example: http://stackoverflow.com/questions/11629966/how-to-handle-table-column-named-with-reserved-sql-keyword - Illya Yalovyy On Dec. 13, 2016, 5:14 p.m., Dmitry Zagorulkin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54711/ > ----------------------------------------------------------- > > (Updated Dec. 13, 2016, 5:14 p.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > Please review my aproach for HIVE-1555 implementation. any feedback are > welcome. > > > Diffs > ----- > > jdbc-handler/pom.xml PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/JDBCStorageHandler.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridge.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCTypeBridgeUtil.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/db/HiveJDBCVendorBridge.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleJDBCDBWritable.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleTypeBridge.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/db/oracle/HiveOracleVendorBridge.java > PRE-CREATION > jdbc-handler/src/java/org/apache/hive/storagehandler/serde/JDBCSerde.java > PRE-CREATION > > jdbc-handler/src/java/org/apache/hive/storagehandler/serde/TypeHiveJDBCConversion.java > PRE-CREATION > > jdbc-handler/src/test/org/apache/hive/storagehandler/serde/HiveOracleJDBCDBWritableTest.java > PRE-CREATION > packaging/pom.xml 76e0cffdcfac4c4c6aed73a1ca479716857cc659 > packaging/src/main/assembly/src.xml > f27911235b995850a444c9640a0e3b2090551665 > pom.xml 3d8fa1a044e9da94efef5bef2e01d9959f3d8e92 > > Diff: https://reviews.apache.org/r/54711/diff/ > > > Testing > ------- > > > Thanks, > > Dmitry Zagorulkin > >
