----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1598/#review1579 -----------------------------------------------------------
Thanks for the patch Joey. A high-level suggestion - please add validation that stops users from using both the options of --hive-drop-import-delims and the one you are introducing as they are logically incompatible. A refactoring suggestion and minor checkstyle comments below. src/java/com/cloudera/sqoop/lib/FieldFormatter.java <https://reviews.apache.org/r/1598/#comment3565> It will be better to create another method that is called hiveStringReplaceDelims(String,String) which is called by the original method with replacement string set to empty string. src/java/com/cloudera/sqoop/orm/ClassWriter.java <https://reviews.apache.org/r/1598/#comment3566> Longer than 80. src/java/com/cloudera/sqoop/orm/ClassWriter.java <https://reviews.apache.org/r/1598/#comment3567> Longer than 80. src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/1598/#comment3569> Longer than 80. src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/1598/#comment3568> Longer than 80. src/test/com/cloudera/sqoop/hive/TestHiveImport.java <https://reviews.apache.org/r/1598/#comment3570> Longer than 80. src/test/com/cloudera/sqoop/hive/TestHiveImport.java <https://reviews.apache.org/r/1598/#comment3571> Longer than 80. - Arvind On 2011-08-19 18:52:15, Joey Echeverria wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1598/ > ----------------------------------------------------------- > > (Updated 2011-08-19 18:52:15) > > > Review request for Sqoop. > > > Summary > ------- > > I added a new option, --hive-delims-replacement, which lets you pass in a > replacement string. I did it with a new option to remain backwards compatible > with the existing interface. > > > This addresses bug SQOOP-319. > https://issues.apache.org/jira/browse/SQOOP-319 > > > Diffs > ----- > > src/docs/user/hive-args.txt 7e6b7a0 > src/docs/user/hive.txt 059d7cb > src/java/com/cloudera/sqoop/SqoopOptions.java d760d39 > src/java/com/cloudera/sqoop/lib/FieldFormatter.java 41536e1 > src/java/com/cloudera/sqoop/orm/ClassWriter.java dd3994e > src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 8f629f1 > src/test/com/cloudera/sqoop/hive/TestHiveImport.java 35de2fd > testdata/hive/scripts/fieldWithNewlineReplacementImport.q PRE-CREATION > > Diff: https://reviews.apache.org/r/1598/diff > > > Testing > ------- > > I added a unit test for the new option. I also tested the feature by hand. It > works, but I found a bug when doing --direct (at least with MySQL). It > doesn't end up calling the hiveStringDropDelims() function. Some other kind > of escaping is going on. I'll file that as a separate JIRA. > > > Thanks, > > Joey > >
