You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary via Review Board <no...@reviews.apache.org> on 2018/01/18 11:37:40 UTC
Review Request 65210: HIVE-18480 Create tests for function related
methods
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65210/
-----------------------------------------------------------
Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
Bugs: HIVE-18480
https://issues.apache.org/jira/browse/HIVE-18480
Repository: hive-git
Description
-------
The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
Diffs
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65210/diff/1/
Testing
-------
Run the tests
Thanks,
Peter Vary
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> On Jan. 22, 2018, 1:41 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 409 (patched)
> > <https://reviews.apache.org/r/65210/diff/1/?file=1941480#file1941480line409>
> >
> > After dropping the functions, it could be verified if they are really deleted. Like doing a get or list for the deleted function and check if it doesn't exist any more.
Done, thanks, this caught an error in the test :)
> On Jan. 22, 2018, 1:41 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 455 (patched)
> > <https://reviews.apache.org/r/65210/diff/1/?file=1941480#file1941480line455>
> >
> > What happens if the filter is empty? Is the result going to be empty or contain all functions?
Added a test for this as well. Thanks!
> On Jan. 22, 2018, 1:41 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 544 (patched)
> > <https://reviews.apache.org/r/65210/diff/1/?file=1941480#file1941480line544>
> >
> > What do you mean by changing only the class won't cause exception? In which cases will you get exception during altering a function?
Rewritten the comment :)
I hope this is better now :)
> On Jan. 22, 2018, 1:41 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 605 (patched)
> > <https://reviews.apache.org/r/65210/diff/1/?file=1941480#file1941480line605>
> >
> > Calling alterFunction with null newFunction migh be also a test case.
Done. Thanks!
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65210/#review195897
-----------------------------------------------------------
On Jan. 22, 2018, 2:33 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 2:33 p.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/2/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/#review195897
-----------------------------------------------------------
Thanks a lot Peter for the patch!
It looks good to me, I only have few questions and nits.
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 409 (patched)
<https://reviews.apache.org/r/65210/#comment275246>
After dropping the functions, it could be verified if they are really deleted. Like doing a get or list for the deleted function and check if it doesn't exist any more.
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 455 (patched)
<https://reviews.apache.org/r/65210/#comment275247>
What happens if the filter is empty? Is the result going to be empty or contain all functions?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 544 (patched)
<https://reviews.apache.org/r/65210/#comment275248>
What do you mean by changing only the class won't cause exception? In which cases will you get exception during altering a function?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 605 (patched)
<https://reviews.apache.org/r/65210/#comment275250>
Calling alterFunction with null newFunction migh be also a test case.
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 628 (patched)
<https://reviews.apache.org/r/65210/#comment275249>
I guess the NullPointerException here is a typo. In the catch there is InvalidObjectException.
- Marta Kuczora
On Jan. 18, 2018, 11:37 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 11:37 a.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/1/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/#review195981
-----------------------------------------------------------
Ship it!
Ship It!
- Marta Kuczora
On Jan. 22, 2018, 2:33 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 2:33 p.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/2/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/#review196031
-----------------------------------------------------------
Ship it!
Ship It!
- Adam Szita
On Jan. 23, 2018, 4:47 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2018, 4:47 p.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/3/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/
-----------------------------------------------------------
(Updated Jan. 24, 2018, 10:52 a.m.)
Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
Changes
-------
Addresses Sahil's comments
Bugs: HIVE-18480
https://issues.apache.org/jira/browse/HIVE-18480
Repository: hive-git
Description
-------
The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65210/diff/4/
Changes: https://reviews.apache.org/r/65210/diff/3-4/
Testing
-------
Run the tests
Thanks,
Peter Vary
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
Posted by Sahil Takiar <ta...@gmail.com>.
> On Jan. 23, 2018, 7:50 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 236 (patched)
> > <https://reviews.apache.org/r/65210/diff/3/?file=1944543#file1944543line236>
> >
> > Should we be filing JIRAs for the TODO items?
>
> Peter Vary wrote:
> I have to check the root causes of the issues, and file the jiras accordingly.
> Most probably several of the TODOs will be solved with a 1 or 2 changes.
> After the RCA I will file the jiras under this umbrela jira.
> So if you do not mind, I will do it later
Sounds good to me.
- Sahil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65210/#review196065
-----------------------------------------------------------
On Jan. 24, 2018, 10:52 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2018, 10:52 a.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/4/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> On Jan. 23, 2018, 7:50 p.m., Sahil Takiar wrote:
> > A few comments, in general the tests look very thorough. Nice work!
Thanks for the review Sahil!
> On Jan. 23, 2018, 7:50 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 236 (patched)
> > <https://reviews.apache.org/r/65210/diff/3/?file=1944543#file1944543line236>
> >
> > Should we be filing JIRAs for the TODO items?
I have to check the root causes of the issues, and file the jiras accordingly.
Most probably several of the TODOs will be solved with a 1 or 2 changes.
After the RCA I will file the jiras under this umbrela jira.
So if you do not mind, I will do it later
> On Jan. 23, 2018, 7:50 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 246 (patched)
> > <https://reviews.apache.org/r/65210/diff/3/?file=1944543#file1944543line246>
> >
> > shouldn't this test have an assert somewhere? is it possible to used the `expected` parameter of `@Test` with multiple exceptions?
> >
> > Same with the tests below. I think all unit tests should have an Assert, even if its asserting that a specific exception is being thrown. It helps define the contract for the test.
Yeah, good catch - added the needed asserts.
I did not find a way to test for multiple exceptions with the expected paramter. If you find, I would be happy to use, but did not spend too much time on it, since I expect that these multiple chatches will be removed as soon as we fix the TODO items.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65210/#review196065
-----------------------------------------------------------
On Jan. 24, 2018, 10:52 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2018, 10:52 a.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/4/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
Posted by Sahil Takiar <ta...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65210/#review196065
-----------------------------------------------------------
A few comments, in general the tests look very thorough. Nice work!
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 236 (patched)
<https://reviews.apache.org/r/65210/#comment275585>
Should we be filing JIRAs for the TODO items?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 246 (patched)
<https://reviews.apache.org/r/65210/#comment275586>
shouldn't this test have an assert somewhere? is it possible to used the `expected` parameter of `@Test` with multiple exceptions?
Same with the tests below. I think all unit tests should have an Assert, even if its asserting that a specific exception is being thrown. It helps define the contract for the test.
- Sahil Takiar
On Jan. 23, 2018, 4:47 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2018, 4:47 p.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/3/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/
-----------------------------------------------------------
(Updated Jan. 23, 2018, 4:47 p.m.)
Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
Changes
-------
Addessed Adam's comment
Bugs: HIVE-18480
https://issues.apache.org/jira/browse/HIVE-18480
Repository: hive-git
Description
-------
The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65210/diff/3/
Changes: https://reviews.apache.org/r/65210/diff/2-3/
Testing
-------
Run the tests
Thanks,
Peter Vary
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> On Jan. 23, 2018, 2:07 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
> > Lines 103 (patched)
> > <https://reviews.apache.org/r/65210/diff/2/?file=1943284#file1943284line103>
> >
> > Should we extract "org.apache.hadoop.hive.ql.udf.generic.GenericUDFUpper" into a constant as it is used multiple times here?
Done. Thanks!
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65210/#review196003
-----------------------------------------------------------
On Jan. 23, 2018, 4:47 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2018, 4:47 p.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/3/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/#review196003
-----------------------------------------------------------
Fix it, then Ship it!
Looks good Peter, thanks for the patch!
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java
Lines 103 (patched)
<https://reviews.apache.org/r/65210/#comment275513>
Should we extract "org.apache.hadoop.hive.ql.udf.generic.GenericUDFUpper" into a constant as it is used multiple times here?
- Adam Szita
On Jan. 22, 2018, 2:33 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65210/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2018, 2:33 p.m.)
>
>
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-18480
> https://issues.apache.org/jira/browse/HIVE-18480
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65210/diff/2/
>
>
> Testing
> -------
>
> Run the tests
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 65210: HIVE-18480 Create tests for function related
methods
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/65210/
-----------------------------------------------------------
(Updated Jan. 22, 2018, 2:33 p.m.)
Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar.
Changes
-------
Addressed Marta's comments, and fixed checkstyle errors
Bugs: HIVE-18480
https://issues.apache.org/jira/browse/HIVE-18480
Repository: hive-git
Description
-------
The test tries to go throught all af the function related tests and test with various imputs, so not only the happy path, but the edge cases are covered too
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestFunctions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65210/diff/2/
Changes: https://reviews.apache.org/r/65210/diff/1-2/
Testing
-------
Run the tests
Thanks,
Peter Vary