You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abraham Elmahrek <ab...@cloudera.com> on 2015/06/25 03:29:50 UTC
Review Request 35859: SQOOP-1493: Add ability to import/export true
decimal in Avro instead of serializing it to String
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-1493
https://issues.apache.org/jira/browse/SQOOP-1493
Repository: sqoop-trunk
Description
-------
commit 8583077a33956cd2a3abc05f73172210c31994a9
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Jun 19 05:45:25 2015 +0300
SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
:100644 100644 2e3d884... 628491c... M ivy/libraries.properties
:100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
:100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
:100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
:100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
:100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
:100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
:100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
:100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
:100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
Diffs
-----
ivy/libraries.properties 2e3d884
src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
Diff: https://reviews.apache.org/r/35859/diff/
Testing
-------
ant test && manual
Thanks,
Abraham Elmahrek
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On June 25, 2015, 5 a.m., Qian Xu wrote:
> > ivy/libraries.properties, line 21
> > <https://reviews.apache.org/r/35859/diff/1/?file=991750#file991750line21>
> >
> > I'm not sure, if a snapshot artifact is acceptable for sqoop 1.x release.
So, we'll have to change it when Avro 1.8.0 is released. But this feature exists in Avro master only currently. I think it's ok since Sqoop1 is far from a release and Avro is close to a release.
> On June 25, 2015, 5 a.m., Qian Xu wrote:
> > src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java, line 59
> > <https://reviews.apache.org/r/35859/diff/1/?file=991754#file991754line59>
> >
> > In the head avro version, `DEFAULT_DEFLATE_LEVEL` becomes -1. If you use a magic number 1, will it impact the avro internal handling?
I believe the value has been moved/changed around quite a bit: https://github.com/apache/avro/blob/branch-1.5/lang/java/mapred/src/main/java/org/apache/avro/mapred/AvroOutputFormat.java#L56. Keeping it at 1 makes much more sense given the signature of the deflate method: https://github.com/apache/avro/blob/branch-1.7/lang/java/avro/src/main/java/org/apache/avro/file/CodecFactory.java#L48. it expects a value between 1 and 9.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review89314
-----------------------------------------------------------
On June 25, 2015, 6:13 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 6:13 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Qian Xu <qi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review89314
-----------------------------------------------------------
ivy/libraries.properties (line 21)
<https://reviews.apache.org/r/35859/#comment141901>
I'm not sure, if a snapshot artifact is acceptable for sqoop 1.x release.
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java (line 58)
<https://reviews.apache.org/r/35859/#comment141900>
In the head avro version, `DEFAULT_DEFLATE_LEVEL` becomes -1. If you use a magic number 1, will it impact the avro internal handling?
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java (line 148)
<https://reviews.apache.org/r/35859/#comment141899>
Please keep the close bracet in the same line as the other cases.
- Qian Xu
On June 25, 2015, 9:29 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 9:29 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On July 8, 2015, 10:50 p.m., Jarek Cecho wrote:
> > ivy/libraries.properties, line 21
> > <https://reviews.apache.org/r/35859/diff/3/?file=998684#file998684line21>
> >
> > I feel slightly uncomfortable commiting SNAPSHOT dependency to Sqoop 1. Would it make sense to perhaps wait on the release since it should happen soon?
Yeah, why not!
> On July 8, 2015, 10:50 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java, lines 97-99
> > <https://reviews.apache.org/r/35859/diff/3/?file=998690#file998690line97>
> >
> > For education purpose - why is this needed in output committer? Isn't that running in the same VM as the mapper that alredy sets it up?
This is for the import case. When exporting, these are added during the map phase.
> On July 8, 2015, 10:50 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java, lines 178-188
> > <https://reviews.apache.org/r/35859/diff/3/?file=998692#file998692line178>
> >
> > Would be easier to merge those two methods together? (and probably make them static)
+1 on static. Separated because of the comment above.
> On July 8, 2015, 10:50 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/avro/AvroUtil.java, line 197
> > <https://reviews.apache.org/r/35859/diff/3/?file=998685#file998685line197>
> >
> > I'm thinking out lought here - would it make sense to explicitly cast the avroObject to "BigDecimal"? We're making here assumption that the object is BigDecimal which might not be the case (in case of some unforseen issues) and the explicit cast would in that case throw exception early rather then somewhere down to road. I'm not sure what the performance implication of that cast though and hence I'm just brainstorming here.
I guess it's a question of failing early versus later. There might be a slight performance ding since it needs to access run time information about the object and make a type comparison. The cost isn't large, but it exists. I'm not 100% on how this is done in Java, but in C++ RTTI is normally pretty accessible.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review90990
-----------------------------------------------------------
On July 3, 2015, 3:28 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 3, 2015, 3:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad0a24c9fde9e9099d96c28f3c0de07ccd3
> ivy/libraries.properties 2e3d884ae172f4c501d6c7321c30fb62e0da5376
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf6214e45cf1f21be8c6cf6b486b9f35dcfbd
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b4a464a85262adc94ff976a3e8453089de
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c590884e46a434cc4bc35b4c2ba8ca5d345
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a9d5f51a044c40f0ff03b266eb16bc008f
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca410c6492044685efc6c55b45def27a24ed
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e720aed984e310f309c562f109973c26822e
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c166ce3a0462f09543055c888bea4b0058f
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673bc2d39d6cb5240548d098e640cef8d6bf
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4e532be8d906e08d05b23cc5ce0e74f742
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c3cf118c83361a5ccc177ac63d9d4d4560
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80abefc43490e424a136287f31e84f229bf3
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Jarek Cecho <ja...@apache.org>.
> On July 8, 2015, 10:50 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/avro/AvroUtil.java, line 197
> > <https://reviews.apache.org/r/35859/diff/3/?file=998685#file998685line197>
> >
> > I'm thinking out lought here - would it make sense to explicitly cast the avroObject to "BigDecimal"? We're making here assumption that the object is BigDecimal which might not be the case (in case of some unforseen issues) and the explicit cast would in that case throw exception early rather then somewhere down to road. I'm not sure what the performance implication of that cast though and hence I'm just brainstorming here.
>
> Abraham Elmahrek wrote:
> I guess it's a question of failing early versus later. There might be a slight performance ding since it needs to access run time information about the object and make a type comparison. The cost isn't large, but it exists. I'm not 100% on how this is done in Java, but in C++ RTTI is normally pretty accessible.
I honestly don't know the overhead either, so I'll let it up to you. I've just wanted to kick off the discussion here.
> On July 8, 2015, 10:50 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java, lines 97-99
> > <https://reviews.apache.org/r/35859/diff/3/?file=998690#file998690line97>
> >
> > For education purpose - why is this needed in output committer? Isn't that running in the same VM as the mapper that alredy sets it up?
>
> Abraham Elmahrek wrote:
> This is for the import case. When exporting, these are added during the map phase.
Got it, thanks.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review90990
-----------------------------------------------------------
On July 3, 2015, 3:28 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 3, 2015, 3:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad0a24c9fde9e9099d96c28f3c0de07ccd3
> ivy/libraries.properties 2e3d884ae172f4c501d6c7321c30fb62e0da5376
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf6214e45cf1f21be8c6cf6b486b9f35dcfbd
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b4a464a85262adc94ff976a3e8453089de
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c590884e46a434cc4bc35b4c2ba8ca5d345
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a9d5f51a044c40f0ff03b266eb16bc008f
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca410c6492044685efc6c55b45def27a24ed
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e720aed984e310f309c562f109973c26822e
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c166ce3a0462f09543055c888bea4b0058f
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673bc2d39d6cb5240548d098e640cef8d6bf
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4e532be8d906e08d05b23cc5ce0e74f742
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c3cf118c83361a5ccc177ac63d9d4d4560
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80abefc43490e424a136287f31e84f229bf3
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review90990
-----------------------------------------------------------
Thank you for the patch Abe, I like it :) I wasn't able to run it on real cluster due to the dependency on changed avro libraries (silly I know).
ivy/libraries.properties (line 21)
<https://reviews.apache.org/r/35859/#comment144172>
I feel slightly uncomfortable commiting SNAPSHOT dependency to Sqoop 1. Would it make sense to perhaps wait on the release since it should happen soon?
src/java/org/apache/sqoop/avro/AvroUtil.java (line 196)
<https://reviews.apache.org/r/35859/#comment144187>
I'm thinking out lought here - would it make sense to explicitly cast the avroObject to "BigDecimal"? We're making here assumption that the object is BigDecimal which might not be the case (in case of some unforseen issues) and the explicit cast would in that case throw exception early rather then somewhere down to road. I'm not sure what the performance implication of that cast though and hence I'm just brainstorming here.
src/java/org/apache/sqoop/config/ConfigurationConstants.java (line 106)
<https://reviews.apache.org/r/35859/#comment144190>
Assuming that Avro will add support for additional logical data types in the future, would it make sense to have switch for each logical type that would be backward incompatible rather then having one switch for all logical types? I understand that the check is here to preserve backward compatibility, but I'm worried that semantic of this switch will be "if we add a new logical type, you will get backward incompatible behavior".
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java (lines 96 - 98)
<https://reviews.apache.org/r/35859/#comment144191>
For education purpose - why is this needed in output committer? Isn't that running in the same VM as the mapper that alredy sets it up?
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java (lines 178 - 188)
<https://reviews.apache.org/r/35859/#comment144193>
Would be easier to merge those two methods together? (and probably make them static)
Jarcec
- Jarek Cecho
On July 3, 2015, 3:28 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 3, 2015, 3:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad0a24c9fde9e9099d96c28f3c0de07ccd3
> ivy/libraries.properties 2e3d884ae172f4c501d6c7321c30fb62e0da5376
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf6214e45cf1f21be8c6cf6b486b9f35dcfbd
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b4a464a85262adc94ff976a3e8453089de
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c590884e46a434cc4bc35b4c2ba8ca5d345
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a9d5f51a044c40f0ff03b266eb16bc008f
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca410c6492044685efc6c55b45def27a24ed
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e720aed984e310f309c562f109973c26822e
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c166ce3a0462f09543055c888bea4b0058f
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673bc2d39d6cb5240548d098e640cef8d6bf
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4e532be8d906e08d05b23cc5ce0e74f742
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c3cf118c83361a5ccc177ac63d9d4d4560
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80abefc43490e424a136287f31e84f229bf3
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review91573
-----------------------------------------------------------
LICENSE.txt (lines 365 - 368)
<https://reviews.apache.org/r/35859/#comment145009>
If we drop the "borrowed" code from Hive, then we can also drop this note right?
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java (lines 50 - 62)
<https://reviews.apache.org/r/35859/#comment145008>
It seems that this got unused in the last version of the patch?
- Jarek Cecho
On July 13, 2015, 11:40 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 13, 2015, 11:40 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review91578
-----------------------------------------------------------
Ship it!
I'm +1, provided we'll wait on upstream Avro 1.8.0, so that we don't have to depend on snapshot.
- Jarek Cecho
On July 14, 2015, 1:39 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 1:39 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/
-----------------------------------------------------------
(Updated July 14, 2015, 1:39 a.m.)
Review request for Sqoop.
Bugs: SQOOP-1493
https://issues.apache.org/jira/browse/SQOOP-1493
Repository: sqoop-trunk
Description
-------
commit 8583077a33956cd2a3abc05f73172210c31994a9
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Jun 19 05:45:25 2015 +0300
SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
:100644 100644 2e3d884... 628491c... M ivy/libraries.properties
:100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
:100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
:100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
:100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
:100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
:100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
:100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
:100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
:100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
Diffs (updated)
-----
LICENSE.txt c36c7ad
ivy/libraries.properties 2e3d884
src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
Diff: https://reviews.apache.org/r/35859/diff/
Testing
-------
ant test && manual
Thanks,
Abraham Elmahrek
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/
-----------------------------------------------------------
(Updated July 13, 2015, 11:40 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1493
https://issues.apache.org/jira/browse/SQOOP-1493
Repository: sqoop-trunk
Description
-------
commit 8583077a33956cd2a3abc05f73172210c31994a9
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Jun 19 05:45:25 2015 +0300
SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
:100644 100644 2e3d884... 628491c... M ivy/libraries.properties
:100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
:100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
:100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
:100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
:100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
:100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
:100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
:100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
:100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
Diffs (updated)
-----
LICENSE.txt c36c7ad
ivy/libraries.properties 2e3d884
src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
Diff: https://reviews.apache.org/r/35859/diff/
Testing
-------
ant test && manual
Thanks,
Abraham Elmahrek
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On July 13, 2015, 3:18 p.m., Jarek Cecho wrote:
> > Overall the patch looks good. I have only one concern - I've tried to load generated avro file into Hive and it seems that it doesn't work. Here is what I've did:
> >
> > 1) Create MySQL table
> >
> > CREATE TABLE `decimals` (
> > `id` int(11) NOT NULL AUTO_INCREMENT,
> > `dec1` decimal(3,2) DEFAULT NULL,
> > PRIMARY KEY (`id`)
> > ) ENGINE=MyISAM AUTO_INCREMENT=3 DEFAULT CHARSET=latin1;
> >
> > 2) Insert some data into it
> >
> > insert into decimals values(2, 1.00);
> > insert into decimals values(1, 0.04);
> > insert into decimals values(3, 5.10);
> > insert into decimals values(4, 3.33);
> >
> > 3) Import data with Sqoop
> >
> > sqoop import --connect jdbc:mysql://mysql/sqoop --username sqoop --password sqoop --table decimals --as-avrodatafile
> >
> > 4) Manually create corresponding table in Hive
> >
> > create table decimals(id int, dec decimal(3,2)) stored as avro;
> >
> > 5) Load data
> >
> > load data inpath '/user/root/decimals' overwrite into table decimals;
> >
> > 6) And finally verify data:
> >
> > hive> select * from decimals;
> > OK
> > 2 NULL
> > 1 NULL
> > 3 NULL
> > 4 NULL
> > Time taken: 0.45 seconds, Fetched: 4 row(s)
> >
> > I would expect to see the proper decimal values, but I'm getting NULLs instead. I'm wondering whether I'm doing something wrong or whether we do have a small bug somewhere (that might not necessarily be on the Sqoop side).
Hmm Hive and Impala might not support "fixed" data types with logical type "decimal". Good catch. I'll change this to bytes.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review91470
-----------------------------------------------------------
On July 11, 2015, 12:28 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 12:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review91470
-----------------------------------------------------------
Overall the patch looks good. I have only one concern - I've tried to load generated avro file into Hive and it seems that it doesn't work. Here is what I've did:
1) Create MySQL table
CREATE TABLE `decimals` (
`id` int(11) NOT NULL AUTO_INCREMENT,
`dec1` decimal(3,2) DEFAULT NULL,
PRIMARY KEY (`id`)
) ENGINE=MyISAM AUTO_INCREMENT=3 DEFAULT CHARSET=latin1;
2) Insert some data into it
insert into decimals values(2, 1.00);
insert into decimals values(1, 0.04);
insert into decimals values(3, 5.10);
insert into decimals values(4, 3.33);
3) Import data with Sqoop
sqoop import --connect jdbc:mysql://mysql/sqoop --username sqoop --password sqoop --table decimals --as-avrodatafile
4) Manually create corresponding table in Hive
create table decimals(id int, dec decimal(3,2)) stored as avro;
5) Load data
load data inpath '/user/root/decimals' overwrite into table decimals;
6) And finally verify data:
hive> select * from decimals;
OK
2 NULL
1 NULL
3 NULL
4 NULL
Time taken: 0.45 seconds, Fetched: 4 row(s)
I would expect to see the proper decimal values, but I'm getting NULLs instead. I'm wondering whether I'm doing something wrong or whether we do have a small bug somewhere (that might not necessarily be on the Sqoop side).
- Jarek Cecho
On July 11, 2015, 12:28 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 12:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> LICENSE.txt c36c7ad
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
> src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/
-----------------------------------------------------------
(Updated July 11, 2015, 12:28 a.m.)
Review request for Sqoop.
Bugs: SQOOP-1493
https://issues.apache.org/jira/browse/SQOOP-1493
Repository: sqoop-trunk
Description
-------
commit 8583077a33956cd2a3abc05f73172210c31994a9
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Jun 19 05:45:25 2015 +0300
SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
:100644 100644 2e3d884... 628491c... M ivy/libraries.properties
:100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
:100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
:100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
:100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
:100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
:100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
:100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
:100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
:100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
Diffs (updated)
-----
LICENSE.txt c36c7ad
ivy/libraries.properties 2e3d884
src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b
src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca4
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
Diff: https://reviews.apache.org/r/35859/diff/
Testing
-------
ant test && manual
Thanks,
Abraham Elmahrek
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/
-----------------------------------------------------------
(Updated July 3, 2015, 3:28 a.m.)
Review request for Sqoop.
Bugs: SQOOP-1493
https://issues.apache.org/jira/browse/SQOOP-1493
Repository: sqoop-trunk
Description
-------
commit 8583077a33956cd2a3abc05f73172210c31994a9
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Jun 19 05:45:25 2015 +0300
SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
:100644 100644 2e3d884... 628491c... M ivy/libraries.properties
:100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
:100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
:100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
:100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
:100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
:100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
:100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
:100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
:100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
Diffs (updated)
-----
LICENSE.txt c36c7ad0a24c9fde9e9099d96c28f3c0de07ccd3
ivy/libraries.properties 2e3d884ae172f4c501d6c7321c30fb62e0da5376
src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf6214e45cf1f21be8c6cf6b486b9f35dcfbd
src/java/org/apache/sqoop/config/ConfigurationConstants.java e19c17b4a464a85262adc94ff976a3e8453089de
src/java/org/apache/sqoop/manager/ConnManager.java d9569c590884e46a434cc4bc35b4c2ba8ca5d345
src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a9d5f51a044c40f0ff03b266eb16bc008f
src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java 0ea5ca410c6492044685efc6c55b45def27a24ed
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e720aed984e310f309c562f109973c26822e
src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c166ce3a0462f09543055c888bea4b0058f
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673bc2d39d6cb5240548d098e640cef8d6bf
src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4e532be8d906e08d05b23cc5ce0e74f742
src/test/com/cloudera/sqoop/TestAvroExport.java 663828c3cf118c83361a5ccc177ac63d9d4d4560
src/test/com/cloudera/sqoop/TestAvroImport.java 260e80abefc43490e424a136287f31e84f229bf3
src/test/org/apache/sqoop/TestAvroImport.java PRE-CREATION
Diff: https://reviews.apache.org/r/35859/diff/
Testing
-------
ant test && manual
Thanks,
Abraham Elmahrek
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On June 29, 2015, 11:04 p.m., Ryan Blue wrote:
> > src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java, line 59
> > <https://reviews.apache.org/r/35859/diff/2/?file=992257#file992257line59>
> >
> > This looks unrelated to the decimal change to me. If it is related, could you add a comment to explain?
I thought this was necessary because DEFLATE_LEVEL = -1. It turns out that this is acceptable. The import, however, has been moved!
> On June 29, 2015, 11:04 p.m., Ryan Blue wrote:
> > src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java, line 49
> > <https://reviews.apache.org/r/35859/diff/2/?file=992259#file992259line49>
> >
> > This looks like what was in Hive. While we don't need to update the copyright because it is all ASF, do we need to update the license file to state that some content came from Hive? I think we do because contributors maintain copyright. Therefore we need to be able to track this addition back to the project it came from.
Good point. I'll make a note in the license file and add a comment.
> On June 29, 2015, 11:04 p.m., Ryan Blue wrote:
> > src/test/com/cloudera/sqoop/TestAvroImport.java, line 126
> > <https://reviews.apache.org/r/35859/diff/2/?file=992262#file992262line126>
> >
> > Is this necessary? If the code runs correctly I thought this would be added by the mapper.
We need this in the test case itself though. We're reading Avro files for validation.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review89814
-----------------------------------------------------------
On June 25, 2015, 6:13 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 6:13 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Ryan Blue <bl...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review89814
-----------------------------------------------------------
Overall, this looks fine to me in terms of the Avro usage. I'm not really sure what the right thing to do for Sqoop is so I'll let a Sqoop committer review that part.
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java (line 58)
<https://reviews.apache.org/r/35859/#comment142607>
This looks unrelated to the decimal change to me. If it is related, could you add a comment to explain?
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java (line 49)
<https://reviews.apache.org/r/35859/#comment142609>
This looks like what was in Hive. While we don't need to update the copyright because it is all ASF, do we need to update the license file to state that some content came from Hive? I think we do because contributors maintain copyright. Therefore we need to be able to track this addition back to the project it came from.
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java (line 178)
<https://reviews.apache.org/r/35859/#comment142613>
It would be better to have an accessor method, getBytesForPrecision(int), that does this lookup. It's minor, but without that method you have to remember every time to subtract one from the precision to get the index, which seems likely to break.
src/test/com/cloudera/sqoop/TestAvroImport.java (line 126)
<https://reviews.apache.org/r/35859/#comment142614>
Is this necessary? If the code runs correctly I thought this would be added by the mapper.
- Ryan Blue
On June 25, 2015, 11:13 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 11:13 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On June 30, 2015, 8:55 p.m., Gwen Shapira wrote:
> > Isn't this a serious upgrade issue?
> >
> > It looks like the data type is set automatically, without any input from users.
> > So if someone has an oozie workflow running a daily Sqoop job, the job will start generating different data after an upgrade - potentially breaking integration with existing MR, Spark or Hive code. AFAIK, schema evolution will not work when data type changes from String to Decimal.
Agree completely. I'll add a configuration to enable/disable this.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review89967
-----------------------------------------------------------
On June 25, 2015, 6:13 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 6:13 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/#review89967
-----------------------------------------------------------
Isn't this a serious upgrade issue?
It looks like the data type is set automatically, without any input from users.
So if someone has an oozie workflow running a daily Sqoop job, the job will start generating different data after an upgrade - potentially breaking integration with existing MR, Spark or Hive code. AFAIK, schema evolution will not work when data type changes from String to Decimal.
- Gwen Shapira
On June 25, 2015, 6:13 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35859/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 6:13 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1493
> https://issues.apache.org/jira/browse/SQOOP-1493
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> commit 8583077a33956cd2a3abc05f73172210c31994a9
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Jun 19 05:45:25 2015 +0300
>
> SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
>
> :100644 100644 2e3d884... 628491c... M ivy/libraries.properties
> :100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
> :100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
> :100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
> :100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
> :100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
> :100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
> :100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
> :100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
> :100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
>
>
> Diffs
> -----
>
> ivy/libraries.properties 2e3d884
> src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
> src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
> src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
> src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
> src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
> src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
> src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
>
> Diff: https://reviews.apache.org/r/35859/diff/
>
>
> Testing
> -------
>
> ant test && manual
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 35859: SQOOP-1493: Add ability to import/export
true decimal in Avro instead of serializing it to String
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35859/
-----------------------------------------------------------
(Updated June 25, 2015, 6:13 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1493
https://issues.apache.org/jira/browse/SQOOP-1493
Repository: sqoop-trunk
Description
-------
commit 8583077a33956cd2a3abc05f73172210c31994a9
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Jun 19 05:45:25 2015 +0300
SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
:100644 100644 2e3d884... 628491c... M ivy/libraries.properties
:100644 100644 ee3cf62... 5970981... M src/java/org/apache/sqoop/avro/AvroUtil.java
:100644 100644 d9569c5... 5650079... M src/java/org/apache/sqoop/manager/ConnManager.java
:100644 100644 20f056a... 76c3458... M src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java
:100644 100644 aed1e72... a4ac46e... M src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java
:100644 100644 ab263c1... b60ee42... M src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper
:100644 100644 2576673... 8490582... M src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
:100644 100644 1c6f7f4... aff8d08... M src/java/org/apache/sqoop/orm/ClassWriter.java
:100644 100644 663828c... 42e36ab... M src/test/com/cloudera/sqoop/TestAvroExport.java
:100644 100644 260e80a... 2f680c8... M src/test/com/cloudera/sqoop/TestAvroImport.java
Diffs (updated)
-----
ivy/libraries.properties 2e3d884
src/java/org/apache/sqoop/avro/AvroUtil.java ee3cf62
src/java/org/apache/sqoop/manager/ConnManager.java d9569c5
src/java/org/apache/sqoop/mapreduce/AvroExportMapper.java 20f056a
src/java/org/apache/sqoop/mapreduce/AvroOutputFormat.java aed1e72
src/java/org/apache/sqoop/mapreduce/GenericRecordExportMapper.java ab263c1
src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 2576673
src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
src/test/com/cloudera/sqoop/TestAvroExport.java 663828c
src/test/com/cloudera/sqoop/TestAvroImport.java 260e80a
Diff: https://reviews.apache.org/r/35859/diff/
Testing
-------
ant test && manual
Thanks,
Abraham Elmahrek