sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jarek Cecho <jar...@apache.org>
Subject Re: Review Request 42776: SQOOP-2797: Sqoop2: Datatypes: Add Blob data type support for Derby
Date Tue, 02 Feb 2016 20:54:28 GMT

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




common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java (line
25)
<https://reviews.apache.org/r/42776/#comment178661>

    Nit: We're usually trying to avoid asterisk imports, so let's not make this change.



common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java (lines
57 - 58)
<https://reviews.apache.org/r/42776/#comment178662>

    Since BLOB is for "binary" data, would it make sense to use the getBytes() method and
compare using bytes rather then casting to string?



common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java (line 26)
<https://reviews.apache.org/r/42776/#comment178663>

    Nit: Asterisk import.



common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java (lines 321
- 325)
<https://reviews.apache.org/r/42776/#comment178664>

    Aren't we missing case for BLOB here?



common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java (lines 335
- 336)
<https://reviews.apache.org/r/42776/#comment178665>

    Why are we skipping Blob?



common-test/src/main/java/org/apache/sqoop/common/test/db/types/DerbyTypeList.java (lines
112 - 118)
<https://reviews.apache.org/r/42776/#comment178666>

    Let's make the whole method throw "Exception" to avoid a need to use try {} catch blocks.



common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java (lines 28 - 29)
<https://reviews.apache.org/r/42776/#comment178667>

    Nit: Asterisk import.



common/src/main/java/org/apache/sqoop/schema/type/Blob.java (line 20)
<https://reviews.apache.org/r/42776/#comment178668>

    What is the difference between Blob and Binary? Aren't they the same?



connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java
(line 20)
<https://reviews.apache.org/r/42776/#comment178669>

    Nit: Asterisk imports.



connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java
(line 20)
<https://reviews.apache.org/r/42776/#comment178670>

    Nit: Asterisk imports.



connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java
(lines 81 - 87)
<https://reviews.apache.org/r/42776/#comment178671>

    Why do we need to encode the BLOB differently?



test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (lines 123 - 128)
<https://reviews.apache.org/r/42776/#comment178672>

    The changes in this file seems independent on the other changes. Can we perhaps submit
them in separate JIRA?


- Jarek Cecho


On Jan. 26, 2016, 4:55 a.m., Colin Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42776/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2016, 4:55 a.m.)
> 
> 
> Review request for Sqoop and Colin Ma.
> 
> 
> Bugs: SQOOP-2797
>     https://issues.apache.org/jira/browse/SQOOP-2797
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Add Blob data type support for Derby
> 
> 
> Diffs
> -----
> 
>   common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java
4e1ef6a 
>   common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java afc5016

>   common-test/src/main/java/org/apache/sqoop/common/test/db/types/DerbyTypeList.java
642651d 
>   common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java 3a3f9e8 
>   common/src/main/java/org/apache/sqoop/schema/type/Blob.java PRE-CREATION 
>   common/src/main/java/org/apache/sqoop/schema/type/ColumnType.java 9e415bf 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java
0235f28 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java
a6ffa7c 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/common/SqoopIDFUtils.java
9b0885a 
>   test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java 74fe29b

> 
> Diff: https://reviews.apache.org/r/42776/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Colin Ma
> 
>


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