Re: Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint ClassCastException

[Jarek Cecho <ja...@apache.org>]

> On March 29, 2015, 8:29 p.m., Jarek Cecho wrote:
> > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java, lines 130-142
> > <https://reviews.apache.org/r/32588/diff/1/?file=908480#file908480line130>
> >
> >     I'm not particularly familiar with this code, so forgive me if my question doesn't make much sense here. It seems to me that we're doing quite a lot  if-elses and castings to simply get Number object to i'ts corresponding string representation. Wouldn't be simpler to just do:
> >     
> >     return obj.toString()?
> 
> Abraham Elmahrek wrote:
>     Great question. This is one I wasn't 100% sure about either unfortunately. I think it depends on what we want to expose to connector developers. I've assumed the connector developers want the framework to make sure the data is correct. This may not be right...
>     
>     Here's the different scenarios I see:
>     1. Object value is an integer.
>     2. Object value is a long.
>     3. Object value is a number, but neither a long nor integer.
>     4. Object value is not a number.
>     
>     The resulting code would be affected by the following variables:
>     1. How we want to handle Long values being passed to Integer sized columns
>     2. How we want to handle non-numeric values being passed
>     
>     This code will do the following:
>     1. Column(Integer), Value(Integer) => value as string
>     2. Column(Integer), Value(Long) => integer bits selected then transformed to string (e.g. Java.lang.Long.MAX_VALUE.intValue() should be -1).
>     3. Column(Integer), Value(String) => validate string is a number (perform #1 or #2 casts) or throw an exception.
>     4. Column(Long), Value(Long) => value as string
>     5. Column(Long), Value(Integer) => value as string
>     6. Column(Long), Value(String) => validate string is a number (perform #4 or #5 casts) or throw an exception.
>     
>     I do see your point and this logic might be misplaced. Perhaps we can move this to a validation layer in due time.

Fair enough, let's create a subsequent JIRA to discuss it further?


- Jarek


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


On March 27, 2015, 10:14 p.m., Abraham Elmahrek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32588/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 10:14 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2213
>     https://issues.apache.org/jira/browse/SQOOP-2213
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> commit 4bee6231efff20cad14303628fc3a18db38ecc6b
> Author: Abraham Elmahrek <ab...@apache.org>
> Date:   Fri Mar 27 12:13:47 2015 -0700
> 
>     SQOOP-2213: Sqoop2: toCSVFixedPoint ClassCastException
> 
> :100644 100644 c460f80... 2a7aa1b... M  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java
> :100644 100644 f99d1af... 0819f92... M  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/common/TestSqoopIDFUtils.java
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java c460f80 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/common/TestSqoopIDFUtils.java f99d1af 
> 
> Diff: https://reviews.apache.org/r/32588/diff/
> 
> 
> Testing
> -------
> 
> mvn test
> 
> 
> Thanks,
> 
> Abraham Elmahrek
> 
>