You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Fero Szabo via Review Board <no...@reviews.apache.org> on 2017/12/06 13:35:49 UTC
Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes
check for Varchar instead of Char
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/
-----------------------------------------------------------
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Bugs: SQOOP-3233
https://issues.apache.org/jira/browse/SQOOP-3233
Repository: sqoop-trunk
Description
-------
- Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
- Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
- Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
Diffs
-----
src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e5
src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e58c1e1c9a0364e61627ac1ef725bea8f9
src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c
src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c79498d9ca45db227904049736b5ea493
Diff: https://reviews.apache.org/r/64371/diff/1/
Testing
-------
Ran unit tests (ant clean test), and third party tests as well successfully.
Thanks,
Fero Szabo
Re: Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes check for Varchar instead of Char
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/#review193133
-----------------------------------------------------------
src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java
Line 403 (original), 403 (patched)
<https://reviews.apache.org/r/64371/#comment271639>
Nice catch!
I think we can make this statement more readable if we extract the condition in the inner parenthesis to another method. That might prevent some future bugs and could eliminate some code duplication as well.
src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java
Lines 413 (patched)
<https://reviews.apache.org/r/64371/#comment271640>
nit: I know this was not originally your code, but can we rename the one letter variable?
- Szabolcs Vasas
On Dec. 6, 2017, 2:13 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64371/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2017, 2:13 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3233
> https://issues.apache.org/jira/browse/SQOOP-3233
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> - Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
> - Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
> - Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e58c1e1c9a0364e61627ac1ef725bea8f9
> src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c79498d9ca45db227904049736b5ea493
>
>
> Diff: https://reviews.apache.org/r/64371/diff/2/
>
>
> Testing
> -------
>
> Ran unit tests (ant clean test), and third party tests as well successfully.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes check for Varchar instead of Char
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/#review192985
-----------------------------------------------------------
Hi Fero,
I think some problem occured during patch generation as every file present two times.
Please correct the diff and upload it again!
Thanks,
Bogi
- Boglarka Egyed
On Dec. 6, 2017, 2:13 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64371/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2017, 2:13 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3233
> https://issues.apache.org/jira/browse/SQOOP-3233
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> - Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
> - Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
> - Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e58c1e1c9a0364e61627ac1ef725bea8f9
> src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c79498d9ca45db227904049736b5ea493
>
>
> Diff: https://reviews.apache.org/r/64371/diff/2/
>
>
> Testing
> -------
>
> Ran unit tests (ant clean test), and third party tests as well successfully.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes check for Varchar instead of Char
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/#review192997
-----------------------------------------------------------
Ship it!
Hi Fero,
Thanks for fixing the patch file.
Your change looks good to me, also the unit and 3rd party tests passed with it.
Thanks for fixing this bug!
Cheers,
Bogi
- Boglarka Egyed
On Dec. 6, 2017, 2:13 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64371/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2017, 2:13 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3233
> https://issues.apache.org/jira/browse/SQOOP-3233
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> - Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
> - Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
> - Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e58c1e1c9a0364e61627ac1ef725bea8f9
> src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c79498d9ca45db227904049736b5ea493
>
>
> Diff: https://reviews.apache.org/r/64371/diff/2/
>
>
> Testing
> -------
>
> Ran unit tests (ant clean test), and third party tests as well successfully.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes check for Varchar instead of Char
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/#review193237
-----------------------------------------------------------
Ship it!
Hi Feró,
Thank you for the fix, I have ran the unit and third party tests successfully so everything looks good.
- Szabolcs Vasas
On Dec. 7, 2017, 5:23 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64371/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 5:23 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3233
> https://issues.apache.org/jira/browse/SQOOP-3233
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> - Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
> - Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
> - Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e5
> src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c
>
>
> Diff: https://reviews.apache.org/r/64371/diff/3/
>
>
> Testing
> -------
>
> Ran unit tests (ant clean test), and third party tests as well successfully.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes check for Varchar instead of Char
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/
-----------------------------------------------------------
(Updated Dec. 7, 2017, 5:23 p.m.)
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Changes
-------
Implemented Szabi's recommendations. Also renamed the bd variable to bigDecimal for clarity.
Bugs: SQOOP-3233
https://issues.apache.org/jira/browse/SQOOP-3233
Repository: sqoop-trunk
Description
-------
- Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
- Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
- Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
Diffs (updated)
-----
src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e5
src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c
Diff: https://reviews.apache.org/r/64371/diff/3/
Changes: https://reviews.apache.org/r/64371/diff/2-3/
Testing
-------
Ran unit tests (ant clean test), and third party tests as well successfully.
Thanks,
Fero Szabo
Re: Review Request 64371: SQOOP-3233:
SqoopHCatImportHelper.convertNumberTypes check for Varchar instead of Char
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64371/
-----------------------------------------------------------
(Updated Dec. 6, 2017, 2:13 p.m.)
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Changes
-------
Corrected diff, as it contained the change twice
Bugs: SQOOP-3233
https://issues.apache.org/jira/browse/SQOOP-3233
Repository: sqoop-trunk
Description
-------
- Corrected the type from VARCHAR to CHAR in SqoopHCatImportHelper.convertNumberTypes, that the ticket description mentions.
- Found another bug in the convertNumberTypes function: missing parentheses in the if clause that checks whether the type of val is BigDecimal. Corrected it.
- Slight refactoring: extracted the BigDecimal and Number related conversion code into two different methods, for better readability.
Diffs (updated)
-----
src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java 1c1ed1e58c1e1c9a0364e61627ac1ef725bea8f9
src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 4686493c79498d9ca45db227904049736b5ea493
Diff: https://reviews.apache.org/r/64371/diff/2/
Changes: https://reviews.apache.org/r/64371/diff/1-2/
Testing
-------
Ran unit tests (ant clean test), and third party tests as well successfully.
Thanks,
Fero Szabo