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

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3581/#review4594
-----------------------------------------------------------
Thanks for the patch, Jarcec!  Suggestions below.
/src/java/org/apache/sqoop/orm/ClassWriter.java
<https://reviews.apache.org/r/3581/#comment10241>

    Would "output.startsWith("_")" be better? Also, a nit: add a space after "if".
- Bilung
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
>
>

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

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

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

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

(Updated 2012-01-25 20:43:00.432171)
Review request for Sqoop and Bilung Lee.
Changes
-------

I've incorporated both changes - I've added comment with full explanation and put space after 'if' statement.
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 (updated)
-----

  /src/java/org/apache/sqoop/orm/ClassWriter.java 1235863
  /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1235863

Diff: https://reviews.apache.org/r/3581/diff
Testing
-------

All tests pass for both hadoopversion=20 and hadoopversion=23.
Thanks,

Jarek

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

(Updated 2012-01-25 22:01:34.593867)
Review request for Sqoop and Bilung Lee.
Changes
-------

I've fixed my explanation comment to fulfill check style policy.
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 (updated)
-----

  /src/java/org/apache/sqoop/orm/ClassWriter.java 1235863
  /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1235863

Diff: https://reviews.apache.org/r/3581/diff
Testing
-------

All tests pass for both hadoopversion=20 and hadoopversion=23.
Thanks,

Jarek

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

Ship it!
Thanks for the quick update!

- Bilung
On 2012-01-25 22:01:34, Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3581/
> -----------------------------------------------------------
>
> (Updated 2012-01-25 22:01:34)
>
>
> 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 1235863
>   /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1235863
>
> Diff: https://reviews.apache.org/r/3581/diff
>
>
> Testing
> -------
>
> All tests pass for both hadoopversion=20 and hadoopversion=23.
>
>
> Thanks,
>
> Jarek
>
>

> On 2012-01-25 22:08:48, Bilung Lee wrote:
> > Thanks for the quick update!

I've just uploaded last patch version to the JIRA, feel free to commit it. Thanks for your patience sir :-)
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3581/#review4607
-----------------------------------------------------------
On 2012-01-25 22:01:34, Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3581/
> -----------------------------------------------------------
>
> (Updated 2012-01-25 22:01:34)
>
>
> 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 1235863
>   /src/test/com/cloudera/sqoop/orm/TestClassWriter.java 1235863
>
> Diff: https://reviews.apache.org/r/3581/diff
>
>
> Testing
> -------
>
> All tests pass for both hadoopversion=20 and hadoopversion=23.
>
>
> Thanks,
>
> Jarek
>
>