sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cheolsoo Park" <cheol...@cloudera.com>
Subject Re: Review Request: SQOOP-724 Support Table hints in Microsoft SQL Server
Date Wed, 28 Nov 2012 20:25:54 GMT

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


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