> On 2012-01-25 19:01:05, Bilung Lee wrote: > > /src/java/org/apache/sqoop/orm/ClassWriter.java, line 226 > > <https://reviews.apache.org/r/3581/diff/1/?file=70230#file70230line226> > > > > Would "output.startsWith("_")" be better? Also, a nit: add a space > > after "if". > > Jarek Cecho wrote: > Hi sir, > thank you very much for your comments. I greatly appreciate your review. > I have notes to both your comments :-) > > I've actually run check style validations prior uploading this patch and > just re-check it now. I'm confident that putting space after 'if' is not > enforced by "ant checkstyle". I would just like to double check with you that > we really do have such code policy and if so, I'll create JIRA to fix that > (and of course will fix my patch). > > I've used "candidate.startsWith" instead of "output.startsWith" on > purpose. My goal was to prepend another '_' only in case that original column > was starting with '_' and preserve all other translation cases for sort of > backward compatibility. Let me show the difference on example from test set: > Column "9isLegalInSql" is translated into "_9isLegalInSql" without my patch > and to same "_9isLegalInSql" with my patch. However it would be translated to > "__9isLegalInSql" (notice that there are two "_" at the begging) if I would > use output.startsWith instead. I do not have any objections if you prefer to > use output.startsWith, I just wanted to explain my reasoning :-) > > Jarcec
Thanks for the detailed explanations! I am ok without the space after "if" since it is not enforced by checkstyle. Just think it would be nice to be as consistent as possible. It makes sense to be backward compatible. Could you please add your reason and example as the comments in the code so it would be easier to know it is on purpose? Thanks! - Bilung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3581/#review4594 ----------------------------------------------------------- On 2012-01-22 08:47:35, Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3581/ > ----------------------------------------------------------- > > (Updated 2012-01-22 08:47:35) > > > Review request for Sqoop and Bilung Lee. > > > Summary > ------- > > I've just tweak code generation to support use case mentioned in the JIRA - > case when both keyword and _keyword columns are present in the same table. > Current code generation code might create many other duplicates because of > dropping of unsupported characters and other transformations. Those advanced > problems might be solved by --query and SQL projections ('column!' AS > column2, 'column?' as column3). > > > This addresses bug SQOOP-430. > https://issues.apache.org/jira/browse/SQOOP-430 > > > Diffs > ----- > > /src/java/org/apache/sqoop/orm/ClassWriter.java 1234456 > /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1234456 > > Diff: https://reviews.apache.org/r/3581/diff > > > Testing > ------- > > All tests pass for both hadoopversion=20 and hadoopversion=23. > > > Thanks, > > Jarek > >
