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 via Review Board <no...@reviews.apache.org> on 2018/01/18 13:11:24 UTC
Review Request 65213: HIVE-18479: Create tests to cover methods for
dropping Partitions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/
-----------------------------------------------------------
Review request for hive, Peter Vary and Adam Szita.
Bugs: HIVE-18479
https://issues.apache.org/jira/browse/HIVE-18479
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List<String>, boolean)
- boolean dropPartition(String, String, List<String>, PartitionDropOptions)
- boolean dropPartition(String, String, String, boolean)
The test covers not just the happy pathes, but the edge cases as well.
Diffs
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65213/diff/1/
Testing
-------
Run the tests
Thanks,
Marta Kuczora
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/#review196254
-----------------------------------------------------------
Ship it!
Ship It!
- Peter Vary
On Jan. 24, 2018, 4:37 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2018, 4:37 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List<String>, boolean)
> - boolean dropPartition(String, String, List<String>, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
>
> The test covers not just the happy pathes, but the edge cases as well.
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65213/diff/4/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/#review196309
-----------------------------------------------------------
Ship it!
Ship It!
- Peter Vary
On Jan. 24, 2018, 4:37 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2018, 4:37 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List<String>, boolean)
> - boolean dropPartition(String, String, List<String>, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
>
> The test covers not just the happy pathes, but the edge cases as well.
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java ed071f8
>
>
> Diff: https://reviews.apache.org/r/65213/diff/5/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/
-----------------------------------------------------------
(Updated Jan. 24, 2018, 4:37 p.m.)
Review request for hive, Peter Vary and Adam Szita.
Changes
-------
Fixed some stylecheck issues.
Bugs: HIVE-18479
https://issues.apache.org/jira/browse/HIVE-18479
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List<String>, boolean)
- boolean dropPartition(String, String, List<String>, PartitionDropOptions)
- boolean dropPartition(String, String, String, boolean)
The test covers not just the happy pathes, but the edge cases as well.
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65213/diff/4/
Changes: https://reviews.apache.org/r/65213/diff/3-4/
Testing
-------
Run the tests
Thanks,
Marta Kuczora
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/
-----------------------------------------------------------
(Updated Jan. 23, 2018, 1:57 p.m.)
Review request for hive, Peter Vary and Adam Szita.
Bugs: HIVE-18479
https://issues.apache.org/jira/browse/HIVE-18479
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List<String>, boolean)
- boolean dropPartition(String, String, List<String>, PartitionDropOptions)
- boolean dropPartition(String, String, String, boolean)
The test covers not just the happy pathes, but the edge cases as well.
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65213/diff/3/
Changes: https://reviews.apache.org/r/65213/diff/2-3/
Testing
-------
Run the tests
Thanks,
Marta Kuczora
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/
-----------------------------------------------------------
(Updated Jan. 22, 2018, 12:38 p.m.)
Review request for hive, Peter Vary and Adam Szita.
Changes
-------
Fixed the review findings.
Bugs: HIVE-18479
https://issues.apache.org/jira/browse/HIVE-18479
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List<String>, boolean)
- boolean dropPartition(String, String, List<String>, PartitionDropOptions)
- boolean dropPartition(String, String, String, boolean)
The test covers not just the happy pathes, but the edge cases as well.
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65213/diff/2/
Changes: https://reviews.apache.org/r/65213/diff/1-2/
Testing
-------
Run the tests
Thanks,
Marta Kuczora
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Jan. 19, 2018, 11:50 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 341 (patched)
> > <https://reviews.apache.org/r/65213/diff/1/?file=1941487#file1941487line341>
> >
> > Question: Does this behave the same way for external tables too?
Thanks a lot Peter for the review.
That's a good point. :) No, with external tables, the data won't be deleted.
Added an additional test case where deleteData = purge = true with an external table and the test checks if the partitions is dropped but the location is not deleted.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/#review195809
-----------------------------------------------------------
On Jan. 22, 2018, 12:38 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 12:38 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List<String>, boolean)
> - boolean dropPartition(String, String, List<String>, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
>
> The test covers not just the happy pathes, but the edge cases as well.
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65213/diff/2/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/#review195809
-----------------------------------------------------------
Thanks Marta for the patch.
Just one question.
Peter
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 341 (patched)
<https://reviews.apache.org/r/65213/#comment275134>
Question: Does this behave the same way for external tables too?
- Peter Vary
On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 1:11 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List<String>, boolean)
> - boolean dropPartition(String, String, List<String>, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
>
> The test covers not just the happy pathes, but the edge cases as well.
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65213/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > Overall looks good, I just have small observations.
> >
> > One more nitpick: some helper methods could be static e.g. getYearAndMonthCols
Thanks a lot Adam for the review.
Good point, changed the getYearAndMonthPartCols and getYearPartCol helper methods to be static.
> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 64 (patched)
> > <https://reviews.apache.org/r/65213/diff/1/?file=1941487#file1941487line64>
> >
> > Is there any reason to use 10? Supplying -1 would return all partitions which I think was the original intent in the test cases below (in listPartitions calls).
No specific reason, I just missed this. This is a good point, thanks for it.
> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 165 (patched)
> > <https://reviews.apache.org/r/65213/diff/1/?file=1941487#file1941487line165>
> >
> > createTable is called here twice because it's also in @Before. Does this mean it overrides the existing table with the supplied tableParams?
In this test case I wanted to create a different table with the "auto.purge=true" table parameter. This table has the name "purge_test". In the @Before method, I create the default table with the name "test_drop_part_table". So there will be two tables in this test case.
> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 539-553 (patched)
> > <https://reviews.apache.org/r/65213/diff/1/?file=1941487#file1941487line539>
> >
> > Guava lib has a handy method for these use cases:
> >
> > <E> ArrayList<E> com.google.common.collect.Lists.newArrayList(E... elements)
> >
> > ...which is doing exactly the same + optimising on initial list size.
That's an excellent point, thanks a lot for the hint. I didn't know this lib before.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/#review195796
-----------------------------------------------------------
On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 1:11 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List<String>, boolean)
> - boolean dropPartition(String, String, List<String>, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
>
> The test covers not just the happy pathes, but the edge cases as well.
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65213/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65213: HIVE-18479: Create tests to cover methods
for dropping Partitions
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65213/#review195796
-----------------------------------------------------------
Overall looks good, I just have small observations.
One more nitpick: some helper methods could be static e.g. getYearAndMonthCols
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 64 (patched)
<https://reviews.apache.org/r/65213/#comment275117>
Is there any reason to use 10? Supplying -1 would return all partitions which I think was the original intent in the test cases below (in listPartitions calls).
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 165 (patched)
<https://reviews.apache.org/r/65213/#comment275130>
createTable is called here twice because it's also in @Before. Does this mean it overrides the existing table with the supplied tableParams?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 539-553 (patched)
<https://reviews.apache.org/r/65213/#comment275116>
Guava lib has a handy method for these use cases:
<E> ArrayList<E> com.google.common.collect.Lists.newArrayList(E... elements)
...which is doing exactly the same + optimising on initial list size.
- Adam Szita
On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 1:11 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List<String>, boolean)
> - boolean dropPartition(String, String, List<String>, PartitionDropOptions)
> - boolean dropPartition(String, String, String, boolean)
>
> The test covers not just the happy pathes, but the edge cases as well.
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65213/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>