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 16:58:42 UTC
Review Request 65219: HIVE-18486: Create tests to cover methods for
adding Partitions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65219/
-----------------------------------------------------------
Review request for hive, Peter Vary and Adam Szita.
Bugs: HIVE-18486
https://issues.apache.org/jira/browse/HIVE-18486
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- Partition add_partition(Partition)
- int add_partitions(List<Partition>)
- List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65219/diff/1/
Testing
-------
Run the tests
Thanks,
Marta Kuczora
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > Thanks Marta for the patch.
> > Some questions below.
> > Also I think it would be nice to have test for the default values when adding a partition.
> >
> > Thanks,
> > Peter
Thanks a lot Peter for the review.
That's a good idea, added tests for creating partitions with default values.
> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 236 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line236>
> >
> > There are some interesting validation in the SD. Maybe we can add test for it here too.
Good point, thanks. Created tests for some use cases.
> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 282 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line282>
> >
> > Is it worth to create a test for an External Table too?
Creating partitions for an external table doesn't really differ from creating it for a managed table. But I agree it worth to have a test case for that too. I added it.
> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 700 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line700>
> >
> > Like above - validation inside the SD might be important as well
Created some test cases for sd attribute validations.
> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 749 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line749>
> >
> > Maybe for external table too. And maybe check the locations
Added tests for external table with and without setting the location.
> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 1117 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line1117>
> >
> > I do not understand this check
If the 'metastore.partition.inherit.table.properties' property is set in the metastore config, the partition inherits the listed table parameters.
This property is not set in this test, therefore the partition shouldn't inherit the table parameters. That is what this check is for.
But it is true that this is not obvious, so I added a comment to this check.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65219/#review195811
-----------------------------------------------------------
On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 4:58 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List<Partition>)
> - List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65219/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding 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/65219/#review195811
-----------------------------------------------------------
Thanks Marta for the patch.
Some questions below.
Also I think it would be nice to have test for the default values when adding a partition.
Thanks,
Peter
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 236 (patched)
<https://reviews.apache.org/r/65219/#comment275136>
There are some interesting validation in the SD. Maybe we can add test for it here too.
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 282 (patched)
<https://reviews.apache.org/r/65219/#comment275137>
Is it worth to create a test for an External Table too?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 700 (patched)
<https://reviews.apache.org/r/65219/#comment275138>
Like above - validation inside the SD might be important as well
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 749 (patched)
<https://reviews.apache.org/r/65219/#comment275139>
Maybe for external table too. And maybe check the locations
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 1117 (patched)
<https://reviews.apache.org/r/65219/#comment275135>
I do not understand this check
- Peter Vary
On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 4:58 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List<Partition>)
> - List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65219/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding Partitions
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Jan. 19, 2018, 4:36 p.m., Adam Szita wrote:
> > Looks good! I added small issues to fix, similarly with the dropPartitions change: some helper methods could be static (the ones that don't depend on HMS client certainly) and some could use Lists.newArrayList
Thanks a lot Adam for the review.
I made some helper methods static and also used the Lists.newArrayList where is was needed.
> On Jan. 19, 2018, 4:36 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line68>
> >
> > Again this may be -1 instead, when MAX as a value is not used for any other reason than returning all partitions
No reason, just missed it. Thanks for the hint, fixed it.
> On Jan. 19, 2018, 4:36 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 672 (patched)
> > <https://reviews.apache.org/r/65219/diff/1/?file=1942108#file1942108line672>
> >
> > This is found multiple times in the code, should we create a method for it?
I refactored a bit handling the creation time, so these duplications are eliminated.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65219/#review195825
-----------------------------------------------------------
On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 4:58 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List<Partition>)
> - List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65219/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding 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/65219/#review195825
-----------------------------------------------------------
Looks good! I added small issues to fix, similarly with the dropPartitions change: some helper methods could be static (the ones that don't depend on HMS client certainly) and some could use Lists.newArrayList
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 68 (patched)
<https://reviews.apache.org/r/65219/#comment275161>
Again this may be -1 instead, when MAX as a value is not used for any other reason than returning all partitions
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Lines 672 (patched)
<https://reviews.apache.org/r/65219/#comment275152>
This is found multiple times in the code, should we create a method for it?
- Adam Szita
On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 4:58 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List<Partition>)
> - List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65219/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding 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/65219/#review196256
-----------------------------------------------------------
Ship it!
Ship It!
- Peter Vary
On Jan. 25, 2018, 2:40 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2018, 2:40 p.m.)
>
>
> Review request for hive, Peter Vary and Adam Szita.
>
>
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List<Partition>)
> - List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65219/diff/2/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding 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/65219/
-----------------------------------------------------------
(Updated Jan. 27, 2018, 10:01 a.m.)
Review request for hive, Peter Vary and Adam Szita.
Changes
-------
Fixed the failing TestAddPartitions.testAddPartitionsOneInvalid test. This test was failing due to a concurrency issue. There is already a TODO about this issue, so now just commented out the problematic check and investigate this issue in an other Jira.
Bugs: HIVE-18486
https://issues.apache.org/jira/browse/HIVE-18486
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- Partition add_partition(Partition)
- int add_partitions(List<Partition>)
- List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65219/diff/3/
Changes: https://reviews.apache.org/r/65219/diff/2-3/
Testing
-------
Run the tests
Thanks,
Marta Kuczora
Re: Review Request 65219: HIVE-18486: Create tests to cover methods
for adding 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/65219/
-----------------------------------------------------------
(Updated Jan. 25, 2018, 2:40 p.m.)
Review request for hive, Peter Vary and Adam Szita.
Changes
-------
Addressed the review findings.
- Added tests for creating partitions with default attribute values.
- Added tests for creating partitions without columns and with invalid or incomplete colum infos
- Added tests for creating partitions without serde info
- Added tests for creating partitions for external table with and without location being set
- Made some helper methods static
- Used the List.newArrayList method where it is needed
- Changed the value of the MAX variable to -1
- Removed the creation time checks where it was not necessary
- Fixed some checkstyle issues
Bugs: HIVE-18486
https://issues.apache.org/jira/browse/HIVE-18486
Repository: hive-git
Description
-------
The following methods of IMetaStoreClient are covered by this test.
- Partition add_partition(Partition)
- int add_partitions(List<Partition>)
- List<Partition> add_partitions(List<Partition>, boolean, 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/TestAddPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65219/diff/2/
Changes: https://reviews.apache.org/r/65219/diff/1-2/
Testing
-------
Run the tests
Thanks,
Marta Kuczora