sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szabolcs Vasas <vasas.szabo...@gmail.com>
Subject Re: Review Request 68989: [SQOOP-3387 Include Column-Remarks
Date Thu, 08 Nov 2018 15:05:56 GMT

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


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