Re: Review Request 65210: HIVE-18480 Create tests for function related methods

2018-01-24 Thread Sahil Takiar 


> 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)
> > 
> >
> > 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

2018-01-24 Thread Peter Vary via Review Board 


> 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)
> > 
> >
> > 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)
> > 
> >
> > 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

2018-01-24 Thread Peter Vary via Review Board 

---
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

2018-01-23 Thread Sahil Takiar 

---
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)


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)


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

2018-01-23 Thread Adam Szita via Review Board 

---
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

2018-01-23 Thread Peter Vary via Review Board 


> 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)
> > 
> >
> > 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

2018-01-23 Thread Peter Vary via Review Board 

---
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

2018-01-23 Thread Adam Szita via Review Board 

---
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)


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

2018-01-23 Thread Marta Kuczora via Review Board 

---
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

2018-01-22 Thread Peter Vary via Review Board 


> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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

2018-01-22 Thread Peter Vary via Review Board 

---
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

Re: Review Request 65210: HIVE-18480 Create tests for function related methods

2018-01-22 Thread Marta Kuczora via Review Board 

---
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)


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)


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)


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)


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)


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
> 
>