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/19 15:17:45 UTC

Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index methods

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65241/
-----------------------------------------------------------

Review request for hive, Peter Vary and Adam Szita.


Bugs: HIVE-18496
    https://issues.apache.org/jira/browse/HIVE-18496


Repository: hive-git


Description
-------

The following methods of IMetaStoreClient are covered by this test.
- void createIndex(Index, Table)
- boolean dropIndex(String, String, String, boolean)
- void alter_index(String, String, String, Index)

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/TestAddAlterDropIndexes.java PRE-CREATION 


Diff: https://reviews.apache.org/r/65241/diff/1/


Testing
-------

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index methods

Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.

> On Jan. 22, 2018, 2:21 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
> > Lines 218 (patched)
> > <https://reviews.apache.org/r/65241/diff/1/?file=1942599#file1942599line218>
> >
> >     Is this ok to have this characters in the index name?

Currently it is allowed to add, get and drop indexes with names like this. But for table names, special characters are not allowed, so we should consider restricting the index names like this. I will add a TODO and fix this issue in a separate Jira.


- Marta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65241/#review195904
-----------------------------------------------------------


On Jan. 19, 2018, 3:17 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 3:17 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/1/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index methods

Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.

> On Jan. 22, 2018, 2:21 p.m., Peter Vary wrote:
> > Thanks for the patch Marta! Only one question below

Thanks a lot Peter for the review.


- Marta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65241/#review195904
-----------------------------------------------------------


On Jan. 23, 2018, 5:21 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 5:21 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/2/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index 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/65241/#review195904
-----------------------------------------------------------



Thanks for the patch Marta! Only one question below


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
Lines 218 (patched)
<https://reviews.apache.org/r/65241/#comment275260>

    Is this ok to have this characters in the index name?


- Peter Vary


On Jan. 19, 2018, 3:17 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 3:17 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/1/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index 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/65241/#review196257
-----------------------------------------------------------


Ship it!




Ship It!

- Peter Vary


On Jan. 24, 2018, 3:41 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2018, 3:41 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/3/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index 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/65241/
-----------------------------------------------------------

(Updated Jan. 24, 2018, 3:41 p.m.)


Review request for hive, Peter Vary and Adam Szita.


Changes
-------

Fixed some checkstyle issues and made the checks in the alter test simpler.


Bugs: HIVE-18496
    https://issues.apache.org/jira/browse/HIVE-18496


Repository: hive-git


Description
-------

The following methods of IMetaStoreClient are covered by this test.
- void createIndex(Index, Table)
- boolean dropIndex(String, String, String, boolean)
- void alter_index(String, String, String, Index)

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/TestAddAlterDropIndexes.java PRE-CREATION 


Diff: https://reviews.apache.org/r/65241/diff/3/

Changes: https://reviews.apache.org/r/65241/diff/2-3/


Testing
-------

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index 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/65241/
-----------------------------------------------------------

(Updated Jan. 23, 2018, 5:21 p.m.)


Review request for hive, Peter Vary and Adam Szita.


Changes
-------

Addressed review findings.


Bugs: HIVE-18496
    https://issues.apache.org/jira/browse/HIVE-18496


Repository: hive-git


Description
-------

The following methods of IMetaStoreClient are covered by this test.
- void createIndex(Index, Table)
- boolean dropIndex(String, String, String, boolean)
- void alter_index(String, String, String, Index)

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/TestAddAlterDropIndexes.java PRE-CREATION 


Diff: https://reviews.apache.org/r/65241/diff/2/

Changes: https://reviews.apache.org/r/65241/diff/1-2/


Testing
-------

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index methods

Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.

> On Jan. 22, 2018, 2:12 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
> > Lines 678 (patched)
> > <https://reviews.apache.org/r/65241/diff/1/?file=1942599#file1942599line678>
> >
> >     For handling future bug fixes more easily we could use consistent notation for these TODOs, see line 343.

Fixed it. Thanks for pointing this out.


> On Jan. 22, 2018, 2:12 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
> > Lines 876-880 (patched)
> > <https://reviews.apache.org/r/65241/diff/1/?file=1942599#file1942599line876>
> >
> >     If you'd like to shorten this one, I can recommend something like this:
> >     
> >     cols.stream().collect(
> >       Collectors.toMap(col->col.getName(), col->col)
> >     ).get("id")

Thanks a lot for the hint, I changed the code to be like this.


- Marta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65241/#review195900
-----------------------------------------------------------


On Jan. 19, 2018, 3:17 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 3:17 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/1/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index methods

Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.

> On Jan. 22, 2018, 2:12 p.m., Adam Szita wrote:
> > Thanks for this patch Marta, it looks good! I've made a few recommendations only

Thanks a lot Adam for the review.


- Marta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65241/#review195900
-----------------------------------------------------------


On Jan. 23, 2018, 5:21 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2018, 5:21 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/2/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65241: HIVE-18496: Create tests to cover add/alter/drop index 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/65241/#review195900
-----------------------------------------------------------



Thanks for this patch Marta, it looks good! I've made a few recommendations only


standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
Lines 678 (patched)
<https://reviews.apache.org/r/65241/#comment275254>

    For handling future bug fixes more easily we could use consistent notation for these TODOs, see line 343.



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddAlterDropIndexes.java
Lines 876-880 (patched)
<https://reviews.apache.org/r/65241/#comment275252>

    If you'd like to shorten this one, I can recommend something like this:
    
    cols.stream().collect(
      Collectors.toMap(col->col.getName(), col->col)
    ).get("id")


- Adam Szita


On Jan. 19, 2018, 3:17 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65241/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 3:17 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18496
>     https://issues.apache.org/jira/browse/HIVE-18496
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The following methods of IMetaStoreClient are covered by this test.
> - void createIndex(Index, Table)
> - boolean dropIndex(String, String, String, boolean)
> - void alter_index(String, String, String, Index)
> 
> 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/TestAddAlterDropIndexes.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65241/diff/1/
> 
> 
> Testing
> -------
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>