You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Nick White <nw...@palantir.com> on 2013/07/22 11:31:18 UTC
Review Request 12806: SQOOP-890: ClassWriter Generates setField Casts Based
on the Output Type, not the Input Type
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12806/
-----------------------------------------------------------
Review request for Sqoop.
Repository: sqoop-trunk
Description
-------
Sqoop should be more flexible when converting between number types.
Diffs
-----
src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
src/test/com/cloudera/sqoop/TestAvroExport.java 7016826
Diff: https://reviews.apache.org/r/12806/diff/
Testing
-------
Thanks,
Nick White
Re: Review Request 12806: SQOOP-890: ClassWriter Generates setField Casts
Based on the Output Type, not the Input Type
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12806/#review23728
-----------------------------------------------------------
Hi Nick,
thank you very much for working on this one! I would be interesting to known more about the reasoning for this change. Would you mind describing it a bit more?
I'm having concerns that this patch will allow automatic conversion from Long to Int that might lead to data loss without user knowing that. It also seems that the issue might be easily workarounded by explicitly using --map-java-type argument that will allow the advance users to overcome the issue, but still won't do any implicit conversions.
Jarcec
- Jarek Cecho
On July 22, 2013, 9:38 a.m., Nick White wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12806/
> -----------------------------------------------------------
>
> (Updated July 22, 2013, 9:38 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Sqoop should be more flexible when converting between number types.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
> src/test/com/cloudera/sqoop/TestAvroExport.java 7016826
>
> Diff: https://reviews.apache.org/r/12806/diff/
>
>
> Testing
> -------
>
> Unit test included.
>
>
> Thanks,
>
> Nick White
>
>
Re: Review Request 12806: SQOOP-890: ClassWriter Generates setField Casts
Based on the Output Type, not the Input Type
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12806/#review24046
-----------------------------------------------------------
I totally see your point Nick. I still do not feel entirely comfortable about doing some data conversions without explicit user permission on a stable branch in potentially backward incompatible manner. Do you think that it would make sense to enable this behavior conditionally? We could add a new command line argument allowing "relaxed" and possibly dangerous conversions. This way user have to be explicit that it's acceptable behavior.
- Jarek Cecho
On July 24, 2013, 5:44 p.m., Nick White wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12806/
> -----------------------------------------------------------
>
> (Updated July 24, 2013, 5:44 p.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Sqoop should be more flexible when converting between number types.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
> src/test/com/cloudera/sqoop/TestAvroExport.java 7016826
>
> Diff: https://reviews.apache.org/r/12806/diff/
>
>
> Testing
> -------
>
> Unit test included.
>
>
> Thanks,
>
> Nick White
>
>
Re: Review Request 12806: SQOOP-890: ClassWriter Generates setField Casts
Based on the Output Type, not the Input Type
Posted by Nick White <nw...@palantir.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12806/
-----------------------------------------------------------
(Updated July 24, 2013, 5:44 p.m.)
Review request for Sqoop.
Changes
-------
You're right, it's not essential due to map-columns, but it's really quite convenient when you don't know what types a JDBC driver will map some of the more odd SQL types to (e.g. reals, decimals & numerics - even HSQLDB 1.8 returns doubles for a float column). Sqoop should definitely support silent widening (e.g. storing an int in a SQL long-typed column) but I understand your point re: narrowing. Would a warning when converting be sufficient? Or would you prefer that narrowing types had to be given explicitly on the command line? I don't know any libraries of the top of my head that will narrow (e.g.) a float to a double if that narrowing doesn't result in data loss. I've updated the patch to add a widening and narrowing case, both of which fail in 1.4.3 and pass with the patch.
Thanks -
Repository: sqoop-trunk
Description
-------
Sqoop should be more flexible when converting between number types.
Diffs (updated)
-----
src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
src/test/com/cloudera/sqoop/TestAvroExport.java 7016826
Diff: https://reviews.apache.org/r/12806/diff/
Testing
-------
Unit test included.
Thanks,
Nick White
Re: Review Request 12806: SQOOP-890: ClassWriter Generates setField Casts
Based on the Output Type, not the Input Type
Posted by Nick White <nw...@palantir.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12806/
-----------------------------------------------------------
(Updated July 22, 2013, 9:38 a.m.)
Review request for Sqoop.
Repository: sqoop-trunk
Description
-------
Sqoop should be more flexible when converting between number types.
Diffs
-----
src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
src/test/com/cloudera/sqoop/TestAvroExport.java 7016826
Diff: https://reviews.apache.org/r/12806/diff/
Testing (updated)
-------
Unit test included.
Thanks,
Nick White