You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Andrew Sherman <as...@cloudera.com> on 2017/09/29 16:51:15 UTC
Review Request 62693: HIVE-17635: Add unit tests to
CompactionTxnHandler and use PreparedStatements for queries
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62693/
-----------------------------------------------------------
Review request for hive.
Repository: hive-git
Description
-------
Add a unit test which exercises CompactionTxnHandler.markFailed() and change it to use PreparedStament.
Add test for checkFailedCompactions() and change it to use PreparedStatement
Add a unit test which exercises purgeCompactionHistory().
Add buildQueryWithINClauseStrings() which is suitable for building in clauses for PreparedStatement
Add test code to TestTxnUtils to tickle code in TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
Change markCleaned() to use PreparedStatement
Diffs
-----
beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 84963af10ec13979a7b3976be434efbc21cf2382
metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java 60839faa352cbf959041a455e9e780dfca0afdc3
metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 30b155f3b3311fed6cd79e46a5b2abcee9927d91
metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 1497c00e5dc77c02e53767b014a23e5fd8cb5b29
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java f8ae86bea3fe78374c0e0487d66c661f4f0d78ff
Diff: https://reviews.apache.org/r/62693/diff/1/
Testing
-------
Thanks,
Andrew Sherman
Re: Review Request 62693: HIVE-17635: Add unit tests to
CompactionTxnHandler and use PreparedStatements for queries
Posted by Andrew Sherman via Review Board <no...@reviews.apache.org>.
> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 387 (original), 411 (patched)
> > <https://reviews.apache.org/r/62693/diff/1/?file=1839860#file1839860line412>
> >
> > What is this for?
The 'questions' list is used to create the list containing 'IN list values'. If not using PreparedStatements these would be actual values. In the PreparedStatement case they are a list of ?. This is arguably ugly but it allows us to use common code for PreparedStatements and (unprepared) Statements. See below for a more complete explanantion.
> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 413 (original), 444 (patched)
> > <https://reviews.apache.org/r/62693/diff/1/?file=1839860#file1839860line449>
> >
> > Is this necessary?
No, its a mistake, thanks
> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 432 (original), 475 (patched)
> > <https://reviews.apache.org/r/62693/diff/1/?file=1839860#file1839860line480>
> >
> > If we are changing this, should we just use try-with-resources.
I agree try-with-resources is great, I wanted to mimimize my changes
> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
> > Lines 158 (patched)
> > <https://reviews.apache.org/r/62693/diff/1/?file=1839861#file1839861line158>
> >
> > why is a new return value necessary?
We are looking at code that generates IN clauses:
select count(*) from TXNS where (TXN_ID in (1,2,3,4,5)) and TXN_STATE = 'o'
There are limits on how many values you can have in an IN clause (like maybe 1000), and the code knows something about that.
If you ask it to generate code for a lot of values then it returns multiple queries:
select count(*) from TXNS where (TXN_ID in (1,2,3,4,5)) and TXN_STATE = 'o'
select count(*) from TXNS where (TXN_ID in (1001,1002,1003) and TXN_STATE = 'o'
My change involves using the same logic to build PreparedStatements. These look like:
select count(*) from TXNS where (TXN_ID in (?,?,?,?,?)) and TXN_STATE = 'o'
select count(*) from TXNS where (TXN_ID in (?,?,?)) and TXN_STATE = 'o'
The difference is that with PreparedStatements the code must also subsequently call
pStmt.setLong(paramNum, value)
The right number of times for each query. So the new method buildQueryWithINClauseStrings,
in addition to building the list of queries also returns a corresponding list of the number of ? i
n the the generated in clause.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62693/#review187818
-----------------------------------------------------------
On Sept. 29, 2017, 4:51 p.m., Andrew Sherman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62693/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2017, 4:51 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Add a unit test which exercises CompactionTxnHandler.markFailed() and change it to use PreparedStament.
> Add test for checkFailedCompactions() and change it to use PreparedStatement
> Add a unit test which exercises purgeCompactionHistory().
> Add buildQueryWithINClauseStrings() which is suitable for building in clauses for PreparedStatement
> Add test code to TestTxnUtils to tickle code in TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
> Change markCleaned() to use PreparedStatement
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 84963af10ec13979a7b3976be434efbc21cf2382
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java 60839faa352cbf959041a455e9e780dfca0afdc3
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 30b155f3b3311fed6cd79e46a5b2abcee9927d91
> metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 1497c00e5dc77c02e53767b014a23e5fd8cb5b29
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java f8ae86bea3fe78374c0e0487d66c661f4f0d78ff
>
>
> Diff: https://reviews.apache.org/r/62693/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Sherman
>
>
Re: Review Request 62693: HIVE-17635: Add unit tests to
CompactionTxnHandler and use PreparedStatements for queries
Posted by Sahil Takiar <ta...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62693/#review187818
-----------------------------------------------------------
metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 387 (original), 411 (patched)
<https://reviews.apache.org/r/62693/#comment264865>
What is this for?
metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 413 (original), 444 (patched)
<https://reviews.apache.org/r/62693/#comment264861>
Is this necessary?
metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 432 (original), 475 (patched)
<https://reviews.apache.org/r/62693/#comment264864>
If we are changing this, should we just use try-with-resources.
metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 158 (patched)
<https://reviews.apache.org/r/62693/#comment264869>
why is a new return value necessary?
metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 170 (patched)
<https://reviews.apache.org/r/62693/#comment264868>
nit: extra newline
metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 192 (patched)
<https://reviews.apache.org/r/62693/#comment264867>
nit: delete newline
- Sahil Takiar
On Sept. 29, 2017, 4:51 p.m., Andrew Sherman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62693/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2017, 4:51 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Add a unit test which exercises CompactionTxnHandler.markFailed() and change it to use PreparedStament.
> Add test for checkFailedCompactions() and change it to use PreparedStatement
> Add a unit test which exercises purgeCompactionHistory().
> Add buildQueryWithINClauseStrings() which is suitable for building in clauses for PreparedStatement
> Add test code to TestTxnUtils to tickle code in TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
> Change markCleaned() to use PreparedStatement
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 84963af10ec13979a7b3976be434efbc21cf2382
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java 60839faa352cbf959041a455e9e780dfca0afdc3
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 30b155f3b3311fed6cd79e46a5b2abcee9927d91
> metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 1497c00e5dc77c02e53767b014a23e5fd8cb5b29
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java f8ae86bea3fe78374c0e0487d66c661f4f0d78ff
>
>
> Diff: https://reviews.apache.org/r/62693/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Sherman
>
>