sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bilung Lee" <bleeapa...@gmail.com>
Subject Re: Review Request: SQOOP-451 Add new options for format masks for date, time, and timestamp
Date Fri, 01 Jun 2012 20:46:09 GMT

-----------------------------------------------------------
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
> 
>


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