sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Attila Szabo <mau...@apache.org>
Subject Re: Review Request 55769: Import tables with special characters
Date Wed, 08 Mar 2017 12:40:24 GMT

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


Fix it, then Ship it!




Hey Dimitry, Szabolcs, Anna,

First of all I'd like to thank Dimitry for opening this ticket, and providing a contribution
idea how to solve this problem.
I'd like to also thank to Anna and Szabi to review the code of Dimitry.

Although I'd like to raise a two concerns around:
- I would do this one level above (on the SqoopOptions level) to ensure the mapping reflects
the very same names that the engine will use for internal column name presentation.
- I would also introduce a new cmd line option for this mechanism (e.g. --escapeMappingColumnNames
), thus ensure we do not alter the default behaviour and thus don't have to worry about backward
incompatibility and breaking changes. (maybe later we could alter the default behaviour by
switching the default state of the boolean property)

Please change the implementation accordingly,

Thanks,
Attila

ps: Dimitry! I'd like to kindly ask you to add me back to the reviewers of this ticket, thus
I would be able to spot it on my reviewboard dashboard, thus you don't have to wait such a
long time, to get your ticket reviewed/committed. Many thanks in advance!


src/java/org/apache/sqoop/orm/ClassWriter.java
Lines 292-302 (patched)
<https://reviews.apache.org/r/55769/#comment240479>

    I'm concerned if this method has a good place here.
    
    If the improvement would like to make effect on all of the mapping related identifiers,
then we should solve it on the level of SqoopOptions either.



src/java/org/apache/sqoop/orm/ClassWriter.java
Lines 294-300 (original), 306-315 (patched)
<https://reviews.apache.org/r/55769/#comment240480>

    Regardless the place of the cleanMapColumnJava method, this must be run/executed only
once.
    
    Generating the map N times, where N == number of columns sounds a serious waste of resources
on the master thread.



src/java/org/apache/sqoop/orm/ClassWriter.java
Line 1711 (original), 1725 (patched)
<https://reviews.apache.org/r/55769/#comment240481>

    If we would consider fixing the whole mapping one level above (in the SqoopOptions as
I've suggested before) I think it would make this change unnecessary.


- Attila Szabo


On Jan. 27, 2017, 1:54 p.m., Dmitry Zagorulkin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55769/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2017, 1:54 p.m.)
> 
> 
> Review request for Sqoop, Olivier Lamy and vishnu  s nair.
> 
> 
> Bugs: SQOOP-3123
>     https://issues.apache.org/jira/browse/SQOOP-3123
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> Special characters processing in table and column names
> 
> https://issues.apache.org/jira/browse/SQOOP-3123
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/orm/ClassWriter.java c18a36f3 
>   src/test/com/cloudera/sqoop/TestAvroImport.java 26edd4ce 
> 
> 
> Diff: https://reviews.apache.org/r/55769/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dmitry Zagorulkin
> 
>


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