You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Colin Ma <ju...@intel.com> on 2015/08/05 09:35:38 UTC
Review Request 37120: SQOOP-2460: Refactor repository-common to make
sqoop2 easy to use external DB as repository
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/
-----------------------------------------------------------
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
Refactor the code in repository-common to fix this problem.
Diffs
-----
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepoUtils.java 73293c0
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
Diff: https://reviews.apache.org/r/37120/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/#review94508
-----------------------------------------------------------
Ship it!
For MySQL, backticks are used by default for table or column identifiers. Backticks are necessary only when the identifier is a MySQL reserved keyword, or when the identifier contains whitespace characters or characters beyond a limited set. As for SQOOP, the table or column identifiers don't belong to such cases, so it's fine to make table or column identifiers without wrapped by quotes or backticks. But we should make table or column identifiers still wrapped with double-quotes for Derby and PostgreSQL as double-quotes also means case sensitive for these databases. As confirmed with you offline that we will still use double-quotes for Derby and PostgreSQL, so I'm +1 for this patch.
- Dian Fu
On Aug. 5, 2015, 7:35 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37120/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
> Refactor the code in repository-common to fix this problem.
>
>
> Diffs
> -----
>
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepoUtils.java 73293c0
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
>
> Diff: https://reviews.apache.org/r/37120/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Colin Ma <ju...@intel.com>.
> On Aug. 20, 2015, 1:56 a.m., Abraham Elmahrek wrote:
> > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java, line 21
> > <https://reviews.apache.org/r/37120/diff/2/?file=1035929#file1035929line21>
> >
> > This is really only for JDBC repositories. I don't think we need RepositoryQuery as much as need CommonRepositoryInsertUpdateDeleteSelectQuery. Seems like an interface for no reason.
Thanks for the comments, the patch is updated.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/#review95905
-----------------------------------------------------------
On Aug. 10, 2015, 8:08 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37120/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 8:08 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
> Refactor the code in repository-common to fix this problem.
>
>
> Diffs
> -----
>
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
>
> Diff: https://reviews.apache.org/r/37120/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/#review95905
-----------------------------------------------------------
Ship it!
Seems fine... just kill the interface!
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java (line 21)
<https://reviews.apache.org/r/37120/#comment151106>
This is really only for JDBC repositories. I don't think we need RepositoryQuery as much as need CommonRepositoryInsertUpdateDeleteSelectQuery. Seems like an interface for no reason.
- Abraham Elmahrek
On Aug. 10, 2015, 8:08 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37120/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 8:08 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
> Refactor the code in repository-common to fix this problem.
>
>
> Diffs
> -----
>
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
>
> Diff: https://reviews.apache.org/r/37120/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/
-----------------------------------------------------------
(Updated Aug. 20, 2015, 2:47 a.m.)
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
Refactor the code in repository-common to fix this problem.
Diffs (updated)
-----
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
Diff: https://reviews.apache.org/r/37120/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 8:08 a.m.)
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
Refactor the code in repository-common to fix this problem.
Diffs (updated)
-----
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
Diff: https://reviews.apache.org/r/37120/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Colin Ma <ju...@intel.com>.
> On Aug. 7, 2015, 5:55 a.m., Dian Fu wrote:
> > Hi Colin, thanks for the patch. A few comments as follows:
> > 1) Seems that these three methods aren't used: getTableNameWithoutEscape/getColumnNameWithoutEscape/getConstraintNameWithoutEscape
> > 2) The exsiting tables are created with double-quote. This means that the existing table names are uppercase. If we access existing tables in PostgreSQL without double-quote, PostgreSQL will firstly convert the table names to lowercase and then it won't find these tables.
Thanks for the comments:
for the point 1: Yes, these methods are not used in this patch, and they are for the JIRA SQOOP-2461. I'll add these changes to the patch for SQOOP-2461.
for the point 2: The double-quote is not suitable for some databases, like MySql, and there will be no case-sensitive problem with MySql if table name without double-quote.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/#review94498
-----------------------------------------------------------
On Aug. 5, 2015, 7:35 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37120/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
> Refactor the code in repository-common to fix this problem.
>
>
> Diffs
> -----
>
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepoUtils.java 73293c0
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
>
> Diff: https://reviews.apache.org/r/37120/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 37120: SQOOP-2460: Refactor repository-common to
make sqoop2 easy to use external DB as repository
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37120/#review94498
-----------------------------------------------------------
Hi Colin, thanks for the patch. A few comments as follows:
1) Seems that these three methods aren't used: getTableNameWithoutEscape/getColumnNameWithoutEscape/getConstraintNameWithoutEscape
2) The exsiting tables are created with double-quote. This means that the existing table names are uppercase. If we access existing tables in PostgreSQL without double-quote, PostgreSQL will firstly convert the table names to lowercase and then it won't find these tables.
- Dian Fu
On Aug. 5, 2015, 7:35 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37120/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Currently, in the class CommonRepositoryInsertUpdateDeleteSelectQuery, the tableName and columnName are wrapped with quotation, like "tableName"."columnName". But some db doesn't support this format, like MySql.
> Refactor the code in repository-common to fix this problem.
>
>
> Diffs
> -----
>
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepoUtils.java 73293c0
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java 1b5e2fb
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java 28f5f6a
> repository/repository-common/src/main/java/org/apache/sqoop/repository/common/RepositoryQuery.java PRE-CREATION
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 2ba3384
>
> Diff: https://reviews.apache.org/r/37120/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>