You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Marta Kuczora <ku...@cloudera.com> on 2017/01/13 11:58:49 UTC
Review Request 55498: HIVE-15538: Test HIVE-13884 with more complex
query predicates
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/
-----------------------------------------------------------
Review request for hive and Chaoyu Tang.
Bugs: HIVE-15538
https://issues.apache.org/jira/browse/HIVE-15538
Repository: hive-git
Description
-------
Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
Diffs
-----
data/files/max_partition_test_input.txt PRE-CREATION
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
Diff: https://reviews.apache.org/r/55498/diff/
Testing
-------
The patch contains only a new unit test. Ran the test multiple times successfully.
Thanks,
Marta Kuczora
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Marta Kuczora <ku...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/
-----------------------------------------------------------
(Updated March 31, 2017, 2:52 p.m.)
Review request for hive and Chaoyu Tang.
Changes
-------
Changed the patch to not use additional data file.
Bugs: HIVE-15538
https://issues.apache.org/jira/browse/HIVE-15538
Repository: hive-git
Description
-------
Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
Diffs (updated)
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 80b1e98
Diff: https://reviews.apache.org/r/55498/diff/3/
Changes: https://reviews.apache.org/r/55498/diff/2-3/
Testing
-------
The patch contains only a new unit test. Ran the test multiple times successfully.
Thanks,
Marta Kuczora
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Marta Kuczora <ku...@cloudera.com>.
> On Jan. 27, 2017, 4:22 p.m., Chaoyu Tang wrote:
> > Thanks for the patch and it looks good. However, I have a couple of questions which need your clarifications. Thanks
Thanks a lot for the review.
> On Jan. 27, 2017, 4:22 p.m., Chaoyu Tang wrote:
> > data/files/max_partition_test_input.txt
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/55498/diff/2/?file=1604753#file1604753line1>
> >
> > As I remember there was an upstream discussion, we should try to avoid to add a new test data file?
Oh, ok, I didn't know about that. I will change the test to use an already existing data file.
> On Jan. 27, 2017, 4:22 p.m., Chaoyu Tang wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java
> > Lines 193 (patched)
> > <https://reviews.apache.org/r/55498/diff/2/?file=1604754#file1604754line193>
> >
> > How could you tell from the test directly that the query has been falled back to ORM?
I didn't find a way to explicitly check in the test whether or not the query falls back to ORM. I went through the code and found some rules when the direct SQL cannot be used (for example in the MetaStoreDirectSql.visit method). If the operator LIKE is used or the type of the column/value is invalid or they don't match, direct SQL cannot be used.
Also found that if the query falls back to ORM, the number of the partitions can be fetched by the getNumPartitionsViaOrmFilter method or by the getPartitionNamesPrunedByExprNoTxn method. This logic is in the ObjectStore.getNumPartitionsByExpr method:
if (exprTree != null) {
try {
numPartitions = getNumPartitionsViaOrmFilter(ctx.getTable(), exprTree, true);
} catch (MetaException e) {
numPartitions = null;
}
}
// if numPartitions could not be obtained from ORM filters, then get number partitions names, and count them
if (numPartitions == null) {
List<String> filteredPartNames = new ArrayList<String>();
getPartitionNamesPrunedByExprNoTxn(ctx.getTable(), tempExpr, "", (short) -1, filteredPartNames);
numPartitions = filteredPartNames.size();
}
The number of partitions will be fetched by the getNumPartitionsViaOrmFilter method for example for the "select value from %s where num=30 or num=25" query and by the getPartitionNamesPrunedByExprNoTxn method for the "select value from %s where num between 26 and 31" query.
I checked these rules and then debugged the queries to see if they really go to the expected way.
Do you have any suggestion how I could do this better or how I could check in the test if the query falls back to ORM?
> On Jan. 27, 2017, 4:22 p.m., Chaoyu Tang wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java
> > Lines 230 (patched)
> > <https://reviews.apache.org/r/55498/diff/2/?file=1604754#file1604754line230>
> >
> > HMS filter might support the IN clause, but not the LIKE in query predicate.
Yes, I noticed that the LIKE expression is not pushed to the filter, but I added a test case for it to see if the partition max limit is applied. If you think this test case is not needed, I will remove it.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review163279
-----------------------------------------------------------
On Jan. 13, 2017, 3:26 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 3:26 p.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> data/files/max_partition_test_input.txt PRE-CREATION
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 121b825
>
>
> Diff: https://reviews.apache.org/r/55498/diff/2/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Chaoyu Tang <ct...@gmail.com>.
> On Jan. 27, 2017, 4:22 p.m., Chaoyu Tang wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java
> > Lines 193 (patched)
> > <https://reviews.apache.org/r/55498/diff/2/?file=1604754#file1604754line193>
> >
> > How could you tell from the test directly that the query has been falled back to ORM?
>
> Marta Kuczora wrote:
> I didn't find a way to explicitly check in the test whether or not the query falls back to ORM. I went through the code and found some rules when the direct SQL cannot be used (for example in the MetaStoreDirectSql.visit method). If the operator LIKE is used or the type of the column/value is invalid or they don't match, direct SQL cannot be used.
> Also found that if the query falls back to ORM, the number of the partitions can be fetched by the getNumPartitionsViaOrmFilter method or by the getPartitionNamesPrunedByExprNoTxn method. This logic is in the ObjectStore.getNumPartitionsByExpr method:
>
> if (exprTree != null) {
> try {
> numPartitions = getNumPartitionsViaOrmFilter(ctx.getTable(), exprTree, true);
> } catch (MetaException e) {
> numPartitions = null;
> }
> }
>
> // if numPartitions could not be obtained from ORM filters, then get number partitions names, and count them
> if (numPartitions == null) {
> List<String> filteredPartNames = new ArrayList<String>();
> getPartitionNamesPrunedByExprNoTxn(ctx.getTable(), tempExpr, "", (short) -1, filteredPartNames);
> numPartitions = filteredPartNames.size();
> }
>
> The number of partitions will be fetched by the getNumPartitionsViaOrmFilter method for example for the "select value from %s where num=30 or num=25" query and by the getPartitionNamesPrunedByExprNoTxn method for the "select value from %s where num between 26 and 31" query.
> I checked these rules and then debugged the queries to see if they really go to the expected way.
>
> Do you have any suggestion how I could do this better or how I could check in the test if the query falls back to ORM?
I did not find a better way either :-)
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review163279
-----------------------------------------------------------
On March 31, 2017, 2:52 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 2:52 p.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 80b1e98
>
>
> Diff: https://reviews.apache.org/r/55498/diff/3/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review163279
-----------------------------------------------------------
Thanks for the patch and it looks good. However, I have a couple of questions which need your clarifications. Thanks
data/files/max_partition_test_input.txt (line 1)
<https://reviews.apache.org/r/55498/#comment234711>
As I remember there was an upstream discussion, we should try to avoid to add a new test data file?
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java (line 193)
<https://reviews.apache.org/r/55498/#comment234709>
How could you tell from the test directly that the query has been falled back to ORM?
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java (line 230)
<https://reviews.apache.org/r/55498/#comment234716>
HMS filter might support the IN clause, but not the LIKE in query predicate.
- Chaoyu Tang
On Jan. 13, 2017, 3:26 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 3:26 p.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> data/files/max_partition_test_input.txt PRE-CREATION
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 121b825
>
> Diff: https://reviews.apache.org/r/55498/diff/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Marta Kuczora <ku...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 3:26 p.m.)
Review request for hive and Chaoyu Tang.
Changes
-------
Improved the test according to the review:
- verify the returned values instead of just checking the number of returned records
- extract the exception message in HiveMetaStore to a public constant and use it in the test
Bugs: HIVE-15538
https://issues.apache.org/jira/browse/HIVE-15538
Repository: hive-git
Description
-------
Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
Diffs (updated)
-----
data/files/max_partition_test_input.txt PRE-CREATION
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 121b825
Diff: https://reviews.apache.org/r/55498/diff/
Testing
-------
The patch contains only a new unit test. Ran the test multiple times successfully.
Thanks,
Marta Kuczora
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Marta Kuczora <ku...@cloudera.com>.
> On Jan. 13, 2017, 1:09 p.m., Peter Vary wrote:
> > The patch is nice and clean.
> > The line lenghts does not conform to the accepted 100 character, but this is the only nit I could find :)
> >
> > About the actual queries, I do not know this part of the code enough to help here. Someone with more knowledge is needed there.
> >
> > Thanks for the patch!
Thanks a lot for the review!
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review161514
-----------------------------------------------------------
On Jan. 13, 2017, 3:26 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 3:26 p.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> data/files/max_partition_test_input.txt PRE-CREATION
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 121b825
>
> Diff: https://reviews.apache.org/r/55498/diff/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review161514
-----------------------------------------------------------
The patch is nice and clean.
The line lenghts does not conform to the accepted 100 character, but this is the only nit I could find :)
About the actual queries, I do not know this part of the code enough to help here. Someone with more knowledge is needed there.
Thanks for the patch!
- Peter Vary
On Jan. 13, 2017, 11:58 a.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 11:58 a.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> data/files/max_partition_test_input.txt PRE-CREATION
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55498/diff/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Marta Kuczora <ku...@cloudera.com>.
> On Jan. 13, 2017, 2:22 p.m., Barna Zsombor Klara wrote:
> > Thanks for the patch Marta, +1. And thank you for taking the time to write a unit test for this instead of a qtest.
> > If you can, please take a look at my comments below, but in the current state it is already fine.
Thanks a lot for the review!
> On Jan. 13, 2017, 2:22 p.m., Barna Zsombor Klara wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java, line 288
> > <https://reviews.apache.org/r/55498/diff/1/?file=1604621#file1604621line288>
> >
> > Can we make the original exception message in HiveMetaStore into a public string pattern and use it here to construct the error message? This way if someone edits the message the test should keep working.
Good point! :) I extracted the pattern into a constant in HiveMetaStore and used it in the test.
> On Jan. 13, 2017, 2:22 p.m., Barna Zsombor Klara wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java, line 267
> > <https://reviews.apache.org/r/55498/diff/1/?file=1604621#file1604621line267>
> >
> > Not sure if it can be done easily but could we somehow check the returned values as well? Right now we only know that the query returned the correct nr of records, whether those are the expected values is unclear.
I tried it and it was quite easy, so added the verification of the returned values.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review161524
-----------------------------------------------------------
On Jan. 13, 2017, 3:26 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 3:26 p.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> data/files/max_partition_test_input.txt PRE-CREATION
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 121b825
>
> Diff: https://reviews.apache.org/r/55498/diff/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 55498: HIVE-15538: Test HIVE-13884 with more
complex query predicates
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55498/#review161524
-----------------------------------------------------------
Thanks for the patch Marta, +1. And thank you for taking the time to write a unit test for this instead of a qtest.
If you can, please take a look at my comments below, but in the current state it is already fine.
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java (line 267)
<https://reviews.apache.org/r/55498/#comment232822>
Not sure if it can be done easily but could we somehow check the returned values as well? Right now we only know that the query returned the correct nr of records, whether those are the expected values is unclear.
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java (line 288)
<https://reviews.apache.org/r/55498/#comment232817>
Can we make the original exception message in HiveMetaStore into a public string pattern and use it here to construct the error message? This way if someone edits the message the test should keep working.
- Barna Zsombor Klara
On Jan. 13, 2017, 11:58 a.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55498/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 11:58 a.m.)
>
>
> Review request for hive and Chaoyu Tang.
>
>
> Bugs: HIVE-15538
> https://issues.apache.org/jira/browse/HIVE-15538
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit test for testing HIVE-13884 with more complex queries and hive.metastore.limit.partition.request enabled.
> It covers cases when the query predicates can be pushed down and the number of partitions can be retrieved via directSQL.
> It also covers cases when the number of partitions cannot be retrieved via directSQL, so it falls back to ORM.
>
>
> Diffs
> -----
>
> data/files/max_partition_test_input.txt PRE-CREATION
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestMetaStoreLimitPartitionRequest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55498/diff/
>
>
> Testing
> -------
>
> The patch contains only a new unit test. Ran the test multiple times successfully.
>
>
> Thanks,
>
> Marta Kuczora
>
>