----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1975/#review2279 -----------------------------------------------------------
Changes look good Jarek. Some feedback noted below: /src/docs/user/import.txt <https://reviews.apache.org/r/1975/#comment5253> nit:Trailing whitespace. /src/docs/user/import.txt <https://reviews.apache.org/r/1975/#comment5254> nit:Trailing whitespace. /src/docs/user/import.txt <https://reviews.apache.org/r/1975/#comment5255> nit:Trailing whitespace. /src/java/com/cloudera/sqoop/SqoopOptions.java <https://reviews.apache.org/r/1975/#comment5294> Nice to have: Consider storing these as properties object. Look at connectionParams as an example. /src/java/com/cloudera/sqoop/SqoopOptions.java <https://reviews.apache.org/r/1975/#comment5295> Nice to have: Basic validations here. Perhaps a Class.forName() test to see if the type actually corresponds to an actual object? /src/java/com/cloudera/sqoop/hive/TableDefWriter.java <https://reviews.apache.org/r/1975/#comment5265> Nice to have: Better to reword this to something like "No column by the name found while importing data". The user may not be aware of what a result set actually is. /src/java/com/cloudera/sqoop/orm/ClassWriter.java <https://reviews.apache.org/r/1975/#comment5293> This method needs to be overwritten, otherwise the Avro Datafile format will likely not work. See AvroSchemaGenerator.generate() method for details. - Arvind On 2011-10-02 08:52:10, Jarek Jarcec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1975/ > ----------------------------------------------------------- > > (Updated 2011-10-02 08:52:10) > > > Review request for Sqoop and Arvind Prabhakar. > > > Summary > ------- > > This is not fully featured patch yet, it's more only preview of what I have > in my mind when I created the bug and how would I image to solve it. I would > like to check with community whether this is acceptable solution and if so, > I'll finish the patch. > > Things that are missing and I'll add them if this way will be accepted: > * Tests > * Documentation > * Supporting for type names (so that user don't have to type the integer > constants on command line) > > Any feedback will be greatly appreciated. > > > This addresses bug sqoop-342. > https://issues.apache.org/jira/browse/sqoop-342 > > > Diffs > ----- > > /src/docs/man/codegen-args.txt 1177987 > /src/docs/man/hive-args.txt 1177987 > /src/docs/man/import-args.txt 1177987 > /src/docs/user/codegen-args.txt 1177987 > /src/docs/user/codegen.txt 1177987 > /src/docs/user/hive-args.txt 1177987 > /src/docs/user/import.txt 1177987 > /src/java/com/cloudera/sqoop/SqoopOptions.java 1177987 > /src/java/com/cloudera/sqoop/hive/TableDefWriter.java 1177987 > /src/java/com/cloudera/sqoop/orm/ClassWriter.java 1177987 > /src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 1177987 > /src/test/com/cloudera/sqoop/TestSqoopOptions.java 1177987 > /src/test/com/cloudera/sqoop/hive/TestTableDefWriter.java 1177987 > /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1177987 > > Diff: https://reviews.apache.org/r/1975/diff > > > Testing > ------- > > > Thanks, > > Jarek > >
