sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abraham Elmahrek" <...@cloudera.com>
Subject Re: Review Request 35859: SQOOP-1493: Add ability to import/export true decimal in Avro instead of serializing it to String
Date Wed, 08 Jul 2015 23:21:02 GMT


> 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 <abe@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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message