You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sqoop.apache.org by Cheolsoo Park <ch...@cloudera.com> on 2012/03/17 01:18:04 UTC

Review Request: SQOOP-451 Add new options for format masks for date, time, and timestamp

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

Review request for Sqoop.

Summary
-------

Add new options via which the user can specify format masks for date, time, and timestamp columns:

--date-mask
--time-mask
--timestamp-mask

To manipulate text from/to the DB, I am using SimpleDateFormat.

The changes include:

1) Add format mask options as Sqoop common options.
2) Update ClassWriter so that SimpleDateFormat format() call can be generated in the toString() method.
3) Update ClassWriter so that SimpleDateFormat parse() call can be generated in the __loadFromFields() method.
4) Add new tests for import format to ManagerCompatTest and its subclasses.
5) Add new tests for export parse to TestExport and its subclasses.
6) Introduce regular expressions into OracleExportTest to get rid of try-catch blocks.
(The format mask options do not format direct output from JDBC drivers.)
7) Fix a minor bug in MySQLCompatTest regarding discarded fractional seconds.

This addresses bug SQOOP-451.
https://issues.apache.org/jira/browse/SQOOP-451

Diffs
-----

./src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1301119
./src/java/org/apache/sqoop/SqoopOptions.java 1301119
./src/java/org/apache/sqoop/orm/ClassWriter.java 1301119
./src/java/org/apache/sqoop/tool/BaseSqoopTool.java 1301119
./src/test/com/cloudera/sqoop/TestExport.java 1301119
./src/test/com/cloudera/sqoop/manager/DirectMySQLExportTest.java 1301119
./src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java 1301119
./src/test/com/cloudera/sqoop/manager/MySQLCompatTest.java 1301119
./src/test/com/cloudera/sqoop/manager/OracleCompatTest.java 1301119
./src/test/com/cloudera/sqoop/manager/OracleExportTest.java 1301119
./src/test/com/cloudera/sqoop/testutil/ImportJobTestCase.java 1301119
./src/test/com/cloudera/sqoop/testutil/ManagerCompatTestCase.java 1301119

Diff: https://reviews.apache.org/r/4327/diff

Testing
-------

- Various format mask tests for import jobs are added to ManagerCompatTest.
- Various format mask tests for export jobs are added to TestExport.
- Ran ant test, ant test -Dthirdparty=true, and ant checkstyle.

Thanks,

Cheolsoo

Re: Review Request: SQOOP-451 Add new options for format masks for date, time, and timestamp

Posted by Cheolsoo Park <ch...@cloudera.com>.


> On 2012-06-01 20:46:09, Bilung Lee wrote:
> > Thanks for the work!  Two comments:
> > - Look like you touch the ClassWriter class.  Any backward compatibility concerns?
> > - Could you also include some doc in the user guide for these new options?
> >

Hi Bilung,

Thanks for reviewing my patch. To answer your questions,
1) There is no backward incompatibility because ClassWriter generates the same code as before if the new options are not enabled.
2) I will update user docs.


- Cheolsoo


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


On 2012-06-01 01:57:39, Cheolsoo Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4327/
> -----------------------------------------------------------
> 
> (Updated 2012-06-01 01:57:39)
> 
> 
> Review request for Sqoop, Bilung Lee and Jarek Cecho.
> 
> 
> Summary
> -------
> 
> Add new options via which the user can specify format masks for date, time, and timestamp columns:
> 
> --date-mask
> --time-mask
> --timestamp-mask
> 
> To format text, I am using SimpleDateFormat.
> 
> The changes include:
> 
> 1) Add the format mask options as Sqoop common options.
> 2) Update ClassWriter so that SimpleDateFormat format() call can be generated in the toString() method.
> 3) Update ClassWriter so that SimpleDateFormat parse() call can be generated in the __loadFromFields() method.
> 4) Add new tests for format to ManagerCompatTest.
> 5) Add new tests for parse to TestExport.
> 
> In addition, I removed all try-catch blocks from TestExport that used to handle various date/time formats in Oracle db. Instead, output texts are formatted by SimpleDateFormat so that they are no longer needed.
> 
> The patch is relatively big, but it is because many new tests are added. The actual changes for new options are not big.
> 
> I would be very grateful if someone could review this patch.
> 
> 
> This addresses bug SQOOP-451.
>     https://issues.apache.org/jira/browse/SQOOP-451
> 
> 
> Diffs
> -----
> 
>   /src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1344954 
>   /src/java/org/apache/sqoop/SqoopOptions.java 1344954 
>   /src/java/org/apache/sqoop/orm/ClassWriter.java 1344954 
>   /src/java/org/apache/sqoop/tool/BaseSqoopTool.java 1344954 
>   /src/test/com/cloudera/sqoop/TestExport.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/DirectMySQLExportTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/MySQLCompatTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/OracleCompatTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/OracleExportTest.java 1344954 
>   /src/test/com/cloudera/sqoop/testutil/ManagerCompatTestCase.java 1344954 
> 
> Diff: https://reviews.apache.org/r/4327/diff
> 
> 
> Testing
> -------
> 
> - Tests in ManagerCompatTest ensure that date/time/timestamp data are formatted correctly when being imported.
> - Tests in TestExport ensure that date/time/timestamp texts are parsed correctly when being exported.
> - Ran ant test, ant test -Dthirdparty=true, and ant checkstyle.
> 
> 
> Thanks,
> 
> Cheolsoo
> 
>

Re: Review Request: SQOOP-451 Add new options for format masks for date, time, and timestamp

Posted by Bilung Lee <bl...@gmail.com>.

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


Thanks for the work!  Two comments:
- Look like you touch the ClassWriter class.  Any backward compatibility concerns?
- Could you also include some doc in the user guide for these new options?


- Bilung


On 2012-06-01 01:57:39, Cheolsoo Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4327/
> -----------------------------------------------------------
> 
> (Updated 2012-06-01 01:57:39)
> 
> 
> Review request for Sqoop, Bilung Lee and Jarek Cecho.
> 
> 
> Summary
> -------
> 
> Add new options via which the user can specify format masks for date, time, and timestamp columns:
> 
> --date-mask
> --time-mask
> --timestamp-mask
> 
> To format text, I am using SimpleDateFormat.
> 
> The changes include:
> 
> 1) Add the format mask options as Sqoop common options.
> 2) Update ClassWriter so that SimpleDateFormat format() call can be generated in the toString() method.
> 3) Update ClassWriter so that SimpleDateFormat parse() call can be generated in the __loadFromFields() method.
> 4) Add new tests for format to ManagerCompatTest.
> 5) Add new tests for parse to TestExport.
> 
> In addition, I removed all try-catch blocks from TestExport that used to handle various date/time formats in Oracle db. Instead, output texts are formatted by SimpleDateFormat so that they are no longer needed.
> 
> The patch is relatively big, but it is because many new tests are added. The actual changes for new options are not big.
> 
> I would be very grateful if someone could review this patch.
> 
> 
> This addresses bug SQOOP-451.
>     https://issues.apache.org/jira/browse/SQOOP-451
> 
> 
> Diffs
> -----
> 
>   /src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1344954 
>   /src/java/org/apache/sqoop/SqoopOptions.java 1344954 
>   /src/java/org/apache/sqoop/orm/ClassWriter.java 1344954 
>   /src/java/org/apache/sqoop/tool/BaseSqoopTool.java 1344954 
>   /src/test/com/cloudera/sqoop/TestExport.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/DirectMySQLExportTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/MySQLCompatTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/OracleCompatTest.java 1344954 
>   /src/test/com/cloudera/sqoop/manager/OracleExportTest.java 1344954 
>   /src/test/com/cloudera/sqoop/testutil/ManagerCompatTestCase.java 1344954 
> 
> Diff: https://reviews.apache.org/r/4327/diff
> 
> 
> Testing
> -------
> 
> - Tests in ManagerCompatTest ensure that date/time/timestamp data are formatted correctly when being imported.
> - Tests in TestExport ensure that date/time/timestamp texts are parsed correctly when being exported.
> - Ran ant test, ant test -Dthirdparty=true, and ant checkstyle.
> 
> 
> Thanks,
> 
> Cheolsoo
> 
>

Re: Review Request: SQOOP-451 Add new options for format masks for date, time, and timestamp

Posted by Cheolsoo Park <ch...@cloudera.com>.

(Updated 2012-06-01 01:57:39.578149)

Review request for Sqoop, Bilung Lee and Jarek Cecho.

Changes
-------

Rebased the patch.

Summary
-------

Add new options via which the user can specify format masks for date, time, and timestamp columns:

--date-mask
--time-mask
--timestamp-mask

To format text, I am using SimpleDateFormat.

The changes include:

1) Add the format mask options as Sqoop common options.
2) Update ClassWriter so that SimpleDateFormat format() call can be generated in the toString() method.
3) Update ClassWriter so that SimpleDateFormat parse() call can be generated in the __loadFromFields() method.
4) Add new tests for format to ManagerCompatTest.
5) Add new tests for parse to TestExport.

In addition, I removed all try-catch blocks from TestExport that used to handle various date/time formats in Oracle db. Instead, output texts are formatted by SimpleDateFormat so that they are no longer needed.

The patch is relatively big, but it is because many new tests are added. The actual changes for new options are not big.

I would be very grateful if someone could review this patch.

This addresses bug SQOOP-451.
https://issues.apache.org/jira/browse/SQOOP-451

Diffs (updated)
-----

/src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1344954
/src/java/org/apache/sqoop/SqoopOptions.java 1344954
/src/java/org/apache/sqoop/orm/ClassWriter.java 1344954
/src/java/org/apache/sqoop/tool/BaseSqoopTool.java 1344954
/src/test/com/cloudera/sqoop/TestExport.java 1344954
/src/test/com/cloudera/sqoop/manager/DirectMySQLExportTest.java 1344954
/src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java 1344954
/src/test/com/cloudera/sqoop/manager/MySQLCompatTest.java 1344954
/src/test/com/cloudera/sqoop/manager/OracleCompatTest.java 1344954
/src/test/com/cloudera/sqoop/manager/OracleExportTest.java 1344954
/src/test/com/cloudera/sqoop/testutil/ManagerCompatTestCase.java 1344954

Diff: https://reviews.apache.org/r/4327/diff

Testing
-------

- Tests in ManagerCompatTest ensure that date/time/timestamp data are formatted correctly when being imported.
- Tests in TestExport ensure that date/time/timestamp texts are parsed correctly when being exported.
- Ran ant test, ant test -Dthirdparty=true, and ant checkstyle.

Thanks,

Cheolsoo

Re: Review Request: SQOOP-451 Add new options for format masks for date, time, and timestamp

Posted by Cheolsoo Park <ch...@cloudera.com>.

(Updated 2012-04-11 07:45:48.994371)

Review request for Sqoop.

Changes
-------

Improve the patch.

Summary (updated)
-------

Add new options via which the user can specify format masks for date, time, and timestamp columns:

--date-mask
--time-mask
--timestamp-mask

To format text, I am using SimpleDateFormat.

The changes include:

The patch is relatively big, but it is because many new tests are added. The actual changes for new options are not big.

I would be very grateful if someone could review this patch.

This addresses bug SQOOP-451.
https://issues.apache.org/jira/browse/SQOOP-451

Diffs (updated)
-----

/src/test/com/cloudera/sqoop/testutil/ManagerCompatTestCase.java 1311550
/src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java 1311550
/src/test/com/cloudera/sqoop/manager/MySQLCompatTest.java 1311550
/src/test/com/cloudera/sqoop/manager/OracleCompatTest.java 1311550
/src/test/com/cloudera/sqoop/manager/OracleExportTest.java 1311550
/src/test/com/cloudera/sqoop/manager/DirectMySQLExportTest.java 1311550
/src/test/com/cloudera/sqoop/TestExport.java 1311550
/src/java/org/apache/sqoop/tool/BaseSqoopTool.java 1311550
/src/java/org/apache/sqoop/orm/ClassWriter.java 1311550
/src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1311550
/src/java/org/apache/sqoop/SqoopOptions.java 1311550

Diff: https://reviews.apache.org/r/4327/diff

Testing (updated)
-------

Thanks,

Cheolsoo