----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8203/#review13830 -----------------------------------------------------------
Hi Jarcec, Overall looks good. I have a few minor comments inline. I will commit it once you correct them, and all tests pass. Thanks! src/docs/user/connectors.txt <https://reviews.apache.org/r/8203/#comment29559> shown on table below => shown below src/docs/user/connectors.txt <https://reviews.apache.org/r/8203/#comment29548> PostgresSQL => MS SQL src/docs/user/connectors.txt <https://reviews.apache.org/r/8203/#comment29557> Can you change this as follows? If you need to work with tables that are located in non-default schemas, you can specify schema names via the +\--schema+ argument. Custom schemas are supported for both import and export jobs. For example: src/docs/user/connectors.txt <https://reviews.apache.org/r/8203/#comment29558> Can you change this as follows? Sqoop supports table hints in both import and export jobs. Table hints are used only for queries that move data from/to Microsoft SQL Server, but they cannot be used for meta data queries. You can specify a comma-separated list of table hints in the +\--table-hints+ argument. For example: src/docs/user/connectors.txt <https://reviews.apache.org/r/8203/#comment29560> shown on table below => shown below src/java/org/apache/sqoop/manager/SQLServerManager.java <https://reviews.apache.org/r/8203/#comment29549> Can you put this line together with other com.cloudera.sqoop.* import statements? src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerExportBatchOutputFormat.java <https://reviews.apache.org/r/8203/#comment29552> Please delete ".". src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerExportBatchOutputFormat.java <https://reviews.apache.org/r/8203/#comment29553> Put a space before "{". src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerExportBatchOutputFormat.java <https://reviews.apache.org/r/8203/#comment29551> Can you put an empty space after ")" ? src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerInputFormat.java <https://reviews.apache.org/r/8203/#comment29554> DB2-specific => MS SQL-specific src/test/com/cloudera/sqoop/manager/SQLServerManagerExportManualTest.java <https://reviews.apache.org/r/8203/#comment29562> Can you please add another test case that passes a comma-separated list of options? src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java <https://reviews.apache.org/r/8203/#comment29563> Can you please add another test case that passes a comma-separated list of options? - Cheolsoo Park On Nov. 24, 2012, 5:07 a.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8203/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2012, 5:07 a.m.) > > > Review request for Sqoop. > > > Description > ------- > > I've added support for table hints into Microsoft SQL Connector. > > > This addresses bug SQOOP-724. > https://issues.apache.org/jira/browse/SQOOP-724 > > > Diffs > ----- > > src/docs/user/connectors.txt d912840a519d408d2f6272a0761dda1fdbcc8859 > src/java/org/apache/sqoop/manager/SQLServerManager.java > 51f867953c985094bb0325f4a834d6b30d815c3c > > src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerExportBatchOutputFormat.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerInputFormat.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerRecordReader.java > PRE-CREATION > src/test/com/cloudera/sqoop/manager/SQLServerManagerExportManualTest.java > ac7a934a634e5bd89b4107d607db7327fc83c6b2 > src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java > bf889d00ca3d7c1d35acc3a6ffc7f0a2551b8b53 > > Diff: https://reviews.apache.org/r/8203/diff/ > > > Testing > ------- > > I've added two unit tests that are more checking that new argument is working > then it's actual usage. It's not trivial to test "hints" when SQL Server > might decide not use them. I've also done real life testing on SQL Server > Express. > > > Thanks, > > Jarek Cecho > >
