You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abraham Elmahrek <ab...@cloudera.com> on 2015/03/27 23:14:45 UTC
Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint
ClassCastException
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32588/
-----------------------------------------------------------
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
Re: Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint
ClassCastException
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> 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()?
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.
- Abraham
-----------------------------------------------------------
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
>
>
Re: Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint
ClassCastException
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> 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.
>
> Jarek Cecho wrote:
> Fair enough, let's create a subsequent JIRA to discuss it further?
https://issues.apache.org/jira/browse/SQOOP-2280
- Abraham
-----------------------------------------------------------
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
>
>
Re: Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint
ClassCastException
Posted by 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
>
>
Re: Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint
ClassCastException
Posted by 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.
>
> Jarek Cecho wrote:
> Fair enough, let's create a subsequent JIRA to discuss it further?
>
> Abraham Elmahrek wrote:
> https://issues.apache.org/jira/browse/SQOOP-2280
Thanks!
- 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
>
>
Re: Review Request 32588: SQOOP-2213: Sqoop2: toCSVFixedPoint
ClassCastException
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32588/#review78164
-----------------------------------------------------------
connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java
<https://reviews.apache.org/r/32588/#comment126673>
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()?
- Jarek Cecho
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
>
>