hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From cheng xu <cheng.a...@intel.com>
Subject Re: Review Request 51695: HIVE-5867: JDBC driver and beeline should support executing an initial SQL script
Date Tue, 13 Sep 2016 03:25:54 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51695/#review148640
-----------------------------------------------------------




jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 134)
<https://reviews.apache.org/r/51695/#comment216142>

    Can you move the getter and setter after setupLoginTimeout method?



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 162)
<https://reviews.apache.org/r/51695/#comment216124>

    Add a new line before this line.



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 183)
<https://reviews.apache.org/r/51695/#comment216145>

    We should support it in embeded mode as well.



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 202)
<https://reviews.apache.org/r/51695/#comment216127>

    remove "this."



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 252)
<https://reviews.apache.org/r/51695/#comment216125>

    Use log



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 260)
<https://reviews.apache.org/r/51695/#comment216137>

    Will this cover the following case?
    
    show tables; show
    tables;



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 273)
<https://reviews.apache.org/r/51695/#comment216131>

    Please throw the original exception.



jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java (line 275)
<https://reviews.apache.org/r/51695/#comment216128>

    if (br != null){
      br.close();
    }



jdbc/src/test/org/apache/hive/jdbc/TestInitSQL.java (line 1)
<https://reviews.apache.org/r/51695/#comment216126>

    Please add license header.



jdbc/src/test/org/apache/hive/jdbc/TestInitSQL.java (line 16)
<https://reviews.apache.org/r/51695/#comment216129>

    Why do you need mock?



jdbc/src/test/org/apache/hive/jdbc/TestInitSQL.java (line 18)
<https://reviews.apache.org/r/51695/#comment216140>

    Can you add some negative cases?
    e.g. 
    # show tables; show
    tables



jdbc/src/test/org/apache/hive/jdbc/TestInitSQL.java (line 24)
<https://reviews.apache.org/r/51695/#comment216135>

    L24-L29 is test case related and we should not add them in the setup method.



jdbc/src/test/org/apache/hive/jdbc/TestInitSQL.java (line 37)
<https://reviews.apache.org/r/51695/#comment216136>

    Please remove L37-38 which is debug used only.



jdbc/src/test/resources/init.sql (line 1)
<https://reviews.apache.org/r/51695/#comment216147>

    Please use echo to create some tmp file and remove them after test case completed since
we need to add many more cases like negative cases.


- cheng xu


On Sept. 13, 2016, 10:36 a.m., Jianguo Tian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51695/
> -----------------------------------------------------------
> 
> (Updated Sept. 13, 2016, 10:36 a.m.)
> 
> 
> Review request for hive and cheng xu.
> 
> 
> Bugs: HIVE-5867
>     https://issues.apache.org/jira/browse/HIVE-5867
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-5867: JDBC driver and beeline should support executing an initial SQL script
> 
> 
> Diffs
> -----
> 
>   jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java ad96a6466dd1aadab71fc261f55be4639dcbe2bf

>   jdbc/src/java/org/apache/hive/jdbc/Utils.java 3161566994d6c6e01de9d88a6e87295684619ffa

>   jdbc/src/test/org/apache/hive/jdbc/TestInitSQL.java PRE-CREATION 
>   jdbc/src/test/resources/init.sql PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51695/diff/
> 
> 
> Testing
> -------
> 
> TestInitSQL.java is JUnit test class which will test method initSql() in HiveConnection.java.
> 
> 
> Thanks,
> 
> Jianguo Tian
> 
>


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