sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Erzsebet Szilagyi <erzsebet.liz.szila...@gmail.com>
Subject Re: Review Request 55079: [SQOOP-3096] Add management of the schema in direct netezza import
Date Mon, 02 Jan 2017 14:55:34 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55079/#review160323
-----------------------------------------------------------



Hi Fred,
I've got some comments regarding formatting and one question about escaping.


src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (lines 300 - 301)
<https://reviews.apache.org/r/55079/#comment231476>

    "Please use two space indents, and space instead of tabs." (https://cwiki.apache.org/confluence/display/SQOOP/How+to+Contribute/#HowtoContribute-ProvidingPatches)



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (line 311)
<https://reviews.apache.org/r/55079/#comment231471>

    Please make sure you remove unnecessary white spaces.
    You can easily spot them here in the Review Board with checking the "View Diff" view where
they show up with red highlighting.



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (line 330)
<https://reviews.apache.org/r/55079/#comment231472>

    I know this whitespace is not your doing, but so close to your change it would be nice
if you could include removing it.



src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java (line
83)
<https://reviews.apache.org/r/55079/#comment231479>

    You have implemented a new escaping mechanism escapeTableName. Are you sure escaping is
not needed here too?


I'm also curious if you've considered adding schema support at a higher level, e.g. Import
instead of Direct Import or even just for Netezza in general, as schemas can be used for exports
as well?

Have you considered adding some tests? I understand that we currently don't have tests for
Netezza direct import, but there is a DirectNetezzaExportManualTest for direct export and
also NetezzaImportManualTest for testing import.
We should always strive for increasing testing coverage.

Thank you,
Liz

- Erzsebet Szilagyi


On Jan. 1, 2017, 9:33 p.m., Frédéric Escandell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55079/
> -----------------------------------------------------------
> 
> (Updated Jan. 1, 2017, 9:33 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> SQOOP-3096
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/manager/DirectNetezzaManager.java af15824 
>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java
2efea53 
> 
> Diff: https://reviews.apache.org/r/55079/diff/
> 
> 
> Testing
> -------
> 
> Tested with a schema included in Netezza database (adding -- --schema <schema name>)
to the sqoop import command line
> 
> 
> Thanks,
> 
> Frédéric Escandell
> 
>


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