----------------------------------------------------------- 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 <[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 > ----- > > 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 > >
