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