----------------------------------------------------------- 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 <[email protected]> > 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 > >
