You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2016/02/02 21:54:28 UTC
Re: Review Request 42776: SQOOP-2797: Sqoop2: Datatypes: Add Blob
data type support for Derby
-----------------------------------------------------------
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
>
>
Re: Review Request 42776: SQOOP-2797: Sqoop2: Datatypes: Add Blob
data type support for Derby
Posted by Jarek Cecho <ja...@apache.org>.
> On Feb. 2, 2016, 8:54 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/schema/type/Blob.java, line 20
> > <https://reviews.apache.org/r/42776/diff/2/?file=1221169#file1221169line20>
> >
> > What is the difference between Blob and Binary? Aren't they the same?
>
> Colin Ma wrote:
> They are not same during the process. To extract the data, the Blob should be transformed to binary[] as the following code:
> Blob blob = resultSet.getBlob(i + 1);
> byte[] bytes = blob.getBytes(1, (int)blob.length());
>
> For Binary, it's unnecessary.
Got it. Changing our schema objects is however a huge change and should be done "by the way" on JIRA that is meant to add something to our test infrastructure. Let's take this one to a standalone JIRA and let's properly document (explain) why we are doing that.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42776/#review117465
-----------------------------------------------------------
On Feb. 3, 2016, 7:54 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42776/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:54 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 ae1b60d
> 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/DerbyProvider.java 8f3e434
> 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 fc25100
>
> Diff: https://reviews.apache.org/r/42776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42776: SQOOP-2797: Sqoop2: Datatypes: Add Blob
data type support for Derby
Posted by Colin Ma <ju...@intel.com>.
> On Feb. 2, 2016, 8:54 p.m., Jarek Cecho wrote:
> > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java, lines 343-344
> > <https://reviews.apache.org/r/42776/diff/2/?file=1221166#file1221166line343>
> >
> > Why are we skipping Blob?
For the skipping Blob, because Blob can't be used in where clause directly. Or wrap some function like "where bolbToStrMethod(blob) = 'XXX'", but this is not recommended because the method is database dependent.
For the Blob type test, we always get a clean table, the where clause is not required for the test.
> On Feb. 2, 2016, 8:54 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/schema/type/Blob.java, line 20
> > <https://reviews.apache.org/r/42776/diff/2/?file=1221169#file1221169line20>
> >
> > What is the difference between Blob and Binary? Aren't they the same?
They are not same during the process. To extract the data, the Blob should be transformed to binary[] as the following code:
Blob blob = resultSet.getBlob(i + 1);
byte[] bytes = blob.getBytes(1, (int)blob.length());
For Binary, it's unnecessary.
> On Feb. 2, 2016, 8:54 p.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/util/SqlTypesUtils.java, lines 91-98
> > <https://reviews.apache.org/r/42776/diff/2/?file=1221172#file1221172line91>
> >
> > Why do we need to encode the BLOB differently?
Yes, as the previous comment, when extract the Blob data, there has some specific process as the following:
Blob blob = resultSet.getBlob(i + 1);
byte[] bytes = blob.getBytes(1, (int)blob.length());
> On Feb. 2, 2016, 8:54 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java, lines 123-128
> > <https://reviews.apache.org/r/42776/diff/2/?file=1221174#file1221174line123>
> >
> > The changes in this file seems independent on the other changes. Can we perhaps submit them in separate JIRA?
Ok, I'll put in a separate JIRA.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42776/#review117465
-----------------------------------------------------------
On Feb. 3, 2016, 7:11 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42776/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:11 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/DerbyProvider.java 8f3e434
> 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
>
> Diff: https://reviews.apache.org/r/42776/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>