sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Qian Xu" <qian.a...@intel.com>
Subject Re: Review Request 29849: Sqoop2: isNull handling should be moved to CSVIntermediateDataFormat
Date Thu, 15 Jan 2015 15:25:30 GMT


> On Jan. 14, 2015, 12:34 a.m., Veena Basavaraj wrote:
> > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/matcher/MatcherError.java,
line 24
> > <https://reviews.apache.org/r/29849/diff/3/?file=819035#file819035line24>
> >
> >     pleae clean up 0000 if it is not used
> >     
> >     I am not sure why we need the 0001 checks since MatcherFactory instantiates
these matchers after doing the right check
> 
> Veena Basavaraj wrote:
>     Let me add some more details on why I say the 0001 check is not required.
>     see the code in Matcher constructor, it is really confusing to have some code in
MatcherFactroy and some code in Matcher Constructor, so if both are empty, then it is always
set to some bytearray, so there is no need to do the extra check, it would have been nice
if all this logic was in one place in MatcherFactroy.
>     
>         if (fromSchema.isEmpty() && toSchema.isEmpty()) {
>           this.fromSchema = ByteArraySchema.getInstance();
>           this.toSchema = ByteArraySchema.getInstance();

I really appreciate your carefulness. I also tried to understand the existing code. I imagine
there are three cases: (1) both sides have a schema (jdbc to jdbc), so a NameMatcher is required
(2) only one side has schema (jdbc to kite), so we need direct data copy (3) the case uses
ByteArraySchema, i dont know. 

The Matcher class has been changed recently. I need some time to refresh the patch with some
bold changes next week.


- Qian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29849/#review67879
-----------------------------------------------------------


On Jan. 14, 2015, 12:13 a.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29849/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2015, 12:13 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1988
>     https://issues.apache.org/jira/browse/SQOOP-1988
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/matcher/LocationMatcher.java
d92723e 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/matcher/Matcher.java
36ac5a5 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/matcher/MatcherError.java
577b091 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/matcher/NameMatcher.java
7cbc39f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/matcher/TestLocationMatcher.java
624fa7b 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/matcher/TestNameMatcher.java
76ff0da 
> 
> Diff: https://reviews.apache.org/r/29849/diff/
> 
> 
> Testing
> -------
> 
> Existing tests passed 
> 
> 
> Thanks,
> 
> Qian Xu
> 
>


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