Re: Review Request 68989: [SQOOP-3387 Include Column-Remarks

[Szabolcs Vasas <va...@gmail.com>]

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



Hi Tomas,

Thank you for submitting this improvement, please find my findings below.

Apart from those you should add your test coverage to your code, as for example currently nothing proves that the queries in org.apache.sqoop.manager.SqlManager#getColumnRemarks and org.apache.sqoop.manager.SqlManager#getTableRemark work.
Also it is a bit confusing that this logic was added to SqlManager but according to the description only Oracle is supported.
I think we should either move this implementation down to OracleManager or test it for all the databases.

Regards,
Szabolcs


src/java/org/apache/sqoop/manager/ConnManager.java
Lines 353 (patched)
<https://reviews.apache.org/r/68989/#comment294660>

    nit: TreeMap<String, String> is redundant, you can use the diamond operator here: TreeMap<>



src/java/org/apache/sqoop/manager/SqlManager.java
Lines 303 (patched)
<https://reviews.apache.org/r/68989/#comment294662>

    The log message does not seem to fit this method.



src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java
Lines 537 (patched)
<https://reviews.apache.org/r/68989/#comment295061>

    This method seems to be very similar to getColumnTypes, can we somehow eliminate the duplication?



src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java
Lines 568 (patched)
<https://reviews.apache.org/r/68989/#comment294663>

    This is an unnecessary override.



src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java
Line 110 (original), 113 (patched)
<https://reviews.apache.org/r/68989/#comment295064>

    I am not sure if changing the doc field here is safe as some people could rely or would like to have the original table name kept. 
    What if you just append the table remark to the doc if exists?



src/java/org/apache/sqoop/orm/ClassWriter.java
Lines 1893 (patched)
<https://reviews.apache.org/r/68989/#comment295062>

    IOException is never thrown here



src/java/org/apache/sqoop/orm/ClassWriter.java
Lines 1897 (patched)
<https://reviews.apache.org/r/68989/#comment295063>

    IOException is never thrown here.



src/java/org/apache/sqoop/orm/ClassWriter.java
Lines 1899 (patched)
<https://reviews.apache.org/r/68989/#comment294658>

    nit: please remove the trailing whitespace from this line


- Szabolcs Vasas


On Oct. 11, 2018, 7:23 a.m., Tomas Sebastian Hätälä wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68989/
> -----------------------------------------------------------
> 
> (Updated Oct. 11, 2018, 7:23 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3387
>     https://issues.apache.org/jira/browse/SQOOP-3387
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> In most RDBMS it is possible to enter comments/ remarks for table and view columns. That way a user can obtain additional information regarding the data and how to use it.
> 
> With the avro file format it would be possible to store this information in the schema file using the "doc"-tag. At the moment this is, however, left blanc.
> 
> This patch includes table and column remarks for Oracle DB and Avro
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/manager/ConnManager.java 4c1e8f5 
>   src/java/org/apache/sqoop/manager/SqlManager.java d82332a 
>   src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 95eaacf 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9 
>   src/java/org/apache/sqoop/orm/ClassWriter.java 46d0698 
> 
> 
> Diff: https://reviews.apache.org/r/68989/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tomas Sebastian Hätälä
> 
>