hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Illya Yalovyy <yalov...@amazon.com>
Subject Re: Review Request 54711: HIVE-1555 [currently support only reading primitive types from oracle db]
Date Thu, 15 Dec 2016 06:30:25 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message