sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitry Zagorulkin <dimon...@gmail.com>
Subject Re: Review Request 55769: Import tables with special characters
Date Wed, 15 Mar 2017 11:13:06 GMT


> On March 8, 2017, 12:40 p.m., Attila Szabo wrote:
> > 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!
> 
> Anna Szonyi wrote:
>     Hi Attila,
>     
>     Thanks for adding your comments, it's a great catch for the multiple calls for cleanMapColumnJava,
we hadn't noticed that!
>     However I would disagree with your second comment of introducing a new cmd line option,
as I believe this is a backwards compatible change (also works with the old way of specifying
of C_1 along with the correct C#1) - though we could put this debate behind with a simple
unit/integration test :)
>     
>     Would you say that if Dmitry fixes the cleanMapColumnJava and adds a test to prove
backwards compatibility you would be comfortable committing this change?
>     
>     Thanks,
>     /Anna

Attila, Anna, Szabolcs thanks for great comments and review. Could you finally clarify how
i should change the implementation? I agree about multiple calls, i will change it. But i
think adding another option will bring some confuse to users because they will should rerun
job with different parameters to get job done.


- Dmitry


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


On March 8, 2017, 8:27 p.m., Dmitry Zagorulkin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55769/
> -----------------------------------------------------------
> 
> (Updated March 8, 2017, 8:27 p.m.)
> 
> 
> Review request for Sqoop, Attila Szabo, 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