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 <ku...@cloudera.com> on 2016/07/27 14:51:46 UTC
Review Request 50503: HIVE-12954: NPE with str_to_map on null strings
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/
-----------------------------------------------------------
Review request for hive, Aihua Xu and Sergio Pena.
Bugs: HIVE-12954
https://issues.apache.org/jira/browse/HIVE-12954
Repository: hive-git
Description
-------
HIVE-12954: NPE with str_to_map on null strings
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
Diff: https://reviews.apache.org/r/50503/diff/
Testing
-------
Added new unit test for GenericUDFStringToMap, please see the patch for details.
Also did manual testing.
Thanks,
Marta Kuczora
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Marta Kuczora <ku...@cloudera.com>.
> On July 27, 2016, 3:26 p.m., Aihua Xu wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java, line 95
> > <https://reviews.apache.org/r/50503/diff/1/?file=1455162#file1455162line95>
> >
> > Are line 90- 93 already dealing with null delimiters ? Seems unnessary to check here?
I could do a unit test which lead to the scenario that the "soi_de1" variable was not null, but the result of "soi_de1.convert(arguments[1].get())" call was null, so the delimiter1 variable ended up being null. This caused a NPE in the split call.
I could reproduce this scenario only from the unit test (testStringToMapWithNullDelimiters test), but not from beeline.
Do you think this scenario can happen outside from the unit test? If not, then these additional null checks are indeed not necessary and I will remove them.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143738
-----------------------------------------------------------
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Zoltan Haindrich <ki...@rxd.hu>.
> On July 27, 2016, 3:26 p.m., Aihua Xu wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java, line 34
> > <https://reviews.apache.org/r/50503/diff/1/?file=1455163#file1455163line34>
> >
> > Please add @Test annotation for unit tests.
if you use @Test annotations then the extends TestCase is not neccessary - if you remove it, you have a junit4 testcase! :)
to get back the assert functions add:
import static org.junit.Assert.*;
> On July 27, 2016, 3:26 p.m., Aihua Xu wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java, line 53
> > <https://reviews.apache.org/r/50503/diff/1/?file=1455163#file1455163line53>
> >
> > Please remove all those empty spaces in this file. :)
there's an eclipse formatter under hive/dev-support ; i think its prepared to remove extra spaces too...might worth a look
- Zoltan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143738
-----------------------------------------------------------
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Marta Kuczora <ku...@cloudera.com>.
> On July 27, 2016, 3:26 p.m., Aihua Xu wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java, line 34
> > <https://reviews.apache.org/r/50503/diff/1/?file=1455163#file1455163line34>
> >
> > Please add @Test annotation for unit tests.
>
> Zoltan Haindrich wrote:
> if you use @Test annotations then the extends TestCase is not neccessary - if you remove it, you have a junit4 testcase! :)
> to get back the assert functions add:
> import static org.junit.Assert.*;
I fixed my test accordingly. Thanks a lot for the hint. :)
> On July 27, 2016, 3:26 p.m., Aihua Xu wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java, line 53
> > <https://reviews.apache.org/r/50503/diff/1/?file=1455163#file1455163line53>
> >
> > Please remove all those empty spaces in this file. :)
>
> Zoltan Haindrich wrote:
> there's an eclipse formatter under hive/dev-support ; i think its prepared to remove extra spaces too...might worth a look
I imported the eclipse-styles.xml file to the formatters in Eclipse, i thought it would remove the extra spaces, but it seems it didn't. I will have a closer look on this, maybe i just missed something in the config.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143738
-----------------------------------------------------------
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143738
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java (line 95)
<https://reviews.apache.org/r/50503/#comment209669>
Are line 90- 93 already dealing with null delimiters ? Seems unnessary to check here?
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java (line 34)
<https://reviews.apache.org/r/50503/#comment209670>
Please add @Test annotation for unit tests.
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java (line 53)
<https://reviews.apache.org/r/50503/#comment209667>
Please remove all those empty spaces in this file. :)
- Aihua Xu
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143742
-----------------------------------------------------------
There are some extra spaces or tabs on the files. Could you remove them?
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java (line 102)
<https://reviews.apache.org/r/50503/#comment209671>
Remove extra spaces here.
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java (line 53)
<https://reviews.apache.org/r/50503/#comment209672>
Remove extra spaces
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java (line 78)
<https://reviews.apache.org/r/50503/#comment209673>
Remove extra spaces
- Sergio Pena
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Marta Kuczora <ku...@cloudera.com>.
> On July 28, 2016, 3:03 p.m., Aihua Xu wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java, line 95
> > <https://reviews.apache.org/r/50503/diff/1/?file=1455162#file1455162line95>
> >
> > I see. You are passing OIs even delimiters are missing and also passing NULLs for delimiters for the unit tests.
> >
> > I think your change is fine since it's possible that we can call like this. You can leave like this.
Thanks a lot for the confirmation. Then i will leave my changes.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143944
-----------------------------------------------------------
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143944
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java (line 95)
<https://reviews.apache.org/r/50503/#comment209888>
I see. You are passing OIs even delimiters are missing and also passing NULLs for delimiters for the unit tests.
I think your change is fine since it's possible that we can call like this. You can leave like this.
- Aihua Xu
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/#review143736
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java (line 102)
<https://reviews.apache.org/r/50503/#comment209665>
Could you remove the trailing space shown as the red blocks.
Patch looks good.
- Chaoyu Tang
On July 27, 2016, 2:51 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50503/
> -----------------------------------------------------------
>
> (Updated July 27, 2016, 2:51 p.m.)
>
>
> Review request for hive, Aihua Xu and Sergio Pena.
>
>
> Bugs: HIVE-12954
> https://issues.apache.org/jira/browse/HIVE-12954
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12954: NPE with str_to_map on null strings
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50503/diff/
>
>
> Testing
> -------
>
> Added new unit test for GenericUDFStringToMap, please see the patch for details.
> Also did manual testing.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 50503: HIVE-12954: NPE with str_to_map on null
strings
Posted by Marta Kuczora <ku...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50503/
-----------------------------------------------------------
(Updated July 29, 2016, 11:01 a.m.)
Review request for hive, Aihua Xu and Sergio Pena.
Changes
-------
Fixed the patch according to the review findings.
Bugs: HIVE-12954
https://issues.apache.org/jira/browse/HIVE-12954
Repository: hive-git
Description
-------
HIVE-12954: NPE with str_to_map on null strings
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFStringToMap.java ed60fbf
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFStringToMap.java PRE-CREATION
Diff: https://reviews.apache.org/r/50503/diff/
Testing
-------
Added new unit test for GenericUDFStringToMap, please see the patch for details.
Also did manual testing.
Thanks,
Marta Kuczora