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