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