You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2015/07/29 19:11:09 UTC
Review Request 36918: SQOOP-2244 Sqoop2: Generic JDBC: Automatically
escape table and column names from configuration objects
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36918/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2244
https://issues.apache.org/jira/browse/SQOOP-2244
Repository: sqoop-sqoop2
Description
-------
The patch is rather big as there has been a lot of things that I had to change. I've changed the semantics on the configuration objects now - we're expecting unescaped table/column names and we're doing the escaping whenever it's needed.
Diffs
-----
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java cab0917
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java 20fabf6
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 4688de3
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/LinkConfiguration.java ed55bff
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/SqlDialect.java PRE-CREATION
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java 22c9e15
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcTestConstants.java 8a5dba4
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 77ac9c3
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestFromInitializer.java 6ae6f90
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java f192c22
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 1c65fc3
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c84e799
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java dac6db7
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 66c016d
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java e9c4543
Diff: https://reviews.apache.org/r/36918/diff/
Testing
-------
Both unit and integration tests are passing.
Thanks,
Jarek Cecho
Re: Review Request 36918: SQOOP-2244 Sqoop2: Generic JDBC:
Automatically escape table and column names from configuration objects
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36918/#review94794
-----------------------------------------------------------
Ship it!
Ship It!
- Abraham Elmahrek
On July 31, 2015, 3:17 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36918/
> -----------------------------------------------------------
>
> (Updated July 31, 2015, 3:17 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2244
> https://issues.apache.org/jira/browse/SQOOP-2244
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The patch is rather big as there has been a lot of things that I had to change. I've changed the semantics on the configuration objects now - we're expecting unescaped table/column names and we're doing the escaping whenever it's needed.
>
>
> Diffs
> -----
>
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java cab0917
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java 20fabf6
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 4688de3
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/LinkConfiguration.java ed55bff
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/SqlDialect.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java 22c9e15
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcTestConstants.java 8a5dba4
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 77ac9c3
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestFromInitializer.java 6ae6f90
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java f192c22
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 1c65fc3
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c84e799
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java dac6db7
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 66c016d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java e9c4543
>
> Diff: https://reviews.apache.org/r/36918/diff/
>
>
> Testing
> -------
>
> Both unit and integration tests are passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 36918: SQOOP-2244 Sqoop2: Generic JDBC:
Automatically escape table and column names from configuration objects
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36918/
-----------------------------------------------------------
(Updated July 31, 2015, 3:17 p.m.)
Review request for Sqoop.
Changes
-------
I've renamed the "escape" to "enclose" as Abe suggested.
Bugs: SQOOP-2244
https://issues.apache.org/jira/browse/SQOOP-2244
Repository: sqoop-sqoop2
Description
-------
The patch is rather big as there has been a lot of things that I had to change. I've changed the semantics on the configuration objects now - we're expecting unescaped table/column names and we're doing the escaping whenever it's needed.
Diffs (updated)
-----
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java cab0917
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java 20fabf6
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 4688de3
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/LinkConfiguration.java ed55bff
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/SqlDialect.java PRE-CREATION
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java 22c9e15
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcTestConstants.java 8a5dba4
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 77ac9c3
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestFromInitializer.java 6ae6f90
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java f192c22
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 1c65fc3
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c84e799
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java dac6db7
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 66c016d
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java e9c4543
Diff: https://reviews.apache.org/r/36918/diff/
Testing
-------
Both unit and integration tests are passing.
Thanks,
Jarek Cecho
Re: Review Request 36918: SQOOP-2244 Sqoop2: Generic JDBC:
Automatically escape table and column names from configuration objects
Posted by Jarek Cecho <ja...@apache.org>.
> On July 30, 2015, 1:41 a.m., Abraham Elmahrek wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java, line 121
> > <https://reviews.apache.org/r/36918/diff/1/?file=1024665#file1024665line121>
> >
> > Default can easily vary based on JDBC url as well. What do you think?
Good point, I should have provide more details.
SQL spec specifies that the default escape character is '"'. I've verified MySQL, Oracle, PostgreSQL and Microsoft SQL Server and they all work correctly with '"' even though thay have their own "natural" escape characters. The only issue is that for MySQL you need to set SQL mode of ANSI_QUOTES [1]. That is also why I've made it configurable, but it will work by default for any database that is following SQL spec which seems fair for Generic JDBC connector :)
Links:
1: https://dev.mysql.com/doc/refman/5.0/en/sql-mode.html#sqlmode_ansi_quotes
> On July 30, 2015, 1:41 a.m., Abraham Elmahrek wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java, line 163
> > <https://reviews.apache.org/r/36918/diff/1/?file=1024665#file1024665line163>
> >
> > Seems like quoting versus escaping.
That is a good point, I'll rename the method.
> On July 30, 2015, 1:41 a.m., Abraham Elmahrek wrote:
> > connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties, line 130
> > <https://reviews.apache.org/r/36918/diff/1/?file=1024670#file1024670line130>
> >
> > Doesn't seem like escaping per-say, but quoting?
That is a good point, I'll rename the method.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36918/#review93537
-----------------------------------------------------------
On July 29, 2015, 5:11 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36918/
> -----------------------------------------------------------
>
> (Updated July 29, 2015, 5:11 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2244
> https://issues.apache.org/jira/browse/SQOOP-2244
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The patch is rather big as there has been a lot of things that I had to change. I've changed the semantics on the configuration objects now - we're expecting unescaped table/column names and we're doing the escaping whenever it's needed.
>
>
> Diffs
> -----
>
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java cab0917
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java 20fabf6
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 4688de3
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/LinkConfiguration.java ed55bff
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/SqlDialect.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java 22c9e15
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcTestConstants.java 8a5dba4
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 77ac9c3
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestFromInitializer.java 6ae6f90
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java f192c22
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 1c65fc3
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c84e799
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java dac6db7
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 66c016d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java e9c4543
>
> Diff: https://reviews.apache.org/r/36918/diff/
>
>
> Testing
> -------
>
> Both unit and integration tests are passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 36918: SQOOP-2244 Sqoop2: Generic JDBC:
Automatically escape table and column names from configuration objects
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36918/#review93537
-----------------------------------------------------------
This seems cool to me, but I think it's more quoting than escaping.
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java (line 120)
<https://reviews.apache.org/r/36918/#comment147923>
Default can easily vary based on JDBC url as well. What do you think?
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java (line 162)
<https://reviews.apache.org/r/36918/#comment147922>
Seems like quoting versus escaping.
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties (line 130)
<https://reviews.apache.org/r/36918/#comment147921>
Doesn't seem like escaping per-say, but quoting?
- Abraham Elmahrek
On July 29, 2015, 5:11 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36918/
> -----------------------------------------------------------
>
> (Updated July 29, 2015, 5:11 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2244
> https://issues.apache.org/jira/browse/SQOOP-2244
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The patch is rather big as there has been a lot of things that I had to change. I've changed the semantics on the configuration objects now - we're expecting unescaped table/column names and we're doing the escaping whenever it's needed.
>
>
> Diffs
> -----
>
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java cab0917
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java 20fabf6
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 4688de3
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/LinkConfiguration.java ed55bff
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/SqlDialect.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java 22c9e15
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcTestConstants.java 8a5dba4
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 77ac9c3
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestFromInitializer.java 6ae6f90
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java f192c22
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 1c65fc3
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c84e799
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java dac6db7
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 66c016d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java e9c4543
>
> Diff: https://reviews.apache.org/r/36918/diff/
>
>
> Testing
> -------
>
> Both unit and integration tests are passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>