> On Jan. 15, 2015, 3:53 p.m., Jarek Cecho wrote: > > common-test/src/main/java/org/apache/sqoop/common/test/db/PostgreSQLProvider.java, > > line 67 > > <https://reviews.apache.org/r/29749/diff/4/?file=819457#file819457line67> > > > > I'm wondering what is the reason to drop the proper escaping? > > Abraham Elmahrek wrote: > PostgreSQL escaping in a nut shell: > http://stackoverflow.com/questions/6331504/omitting-the-double-quote-to-do-query-on-postgresql. > > Essentially, we create tables without any quoting so that the common > repository handler will work. By adding quotes after the fact, it's difficult > to get the correct query. It's much more reasonable to leave the quotes out.
I do see why we would want to stop escaping the table names because we are having inconsistencies in PostgreSQL repository. However I strongly believe that this is a bug in the Repository implementation and it should be fixed there rather then making the strict test provider more relaxed. Otherwise we will very sooon end up having all sort of random errors such as those that we've recently fixed in Sqoop 1 (SQOOP-1890). E.g I think that we should properly escape all table names rather then the other way around. > On Jan. 15, 2015, 3:53 p.m., Jarek Cecho wrote: > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java, > > lines 253-255 > > <https://reviews.apache.org/r/29749/diff/4/?file=819456#file819456line253> > > > > We are not closing the PreparedStatement. > > Abraham Elmahrek wrote: > closeResultSetWithStatement -> rs.getStatement().close() Of course, got it :) - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29749/#review68259 ----------------------------------------------------------- On Jan. 16, 2015, 11:32 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29749/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2015, 11:32 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1591 > https://issues.apache.org/jira/browse/SQOOP-1591 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 560387f1d3e6869ada914fa6ae5ada7d8e210721 > Author: Abraham Elmahrek <[email protected]> > Date: Wed Jan 7 20:44:44 2015 -0800 > > SQOOP-1591: Sqoop2: PostgreSQL integration tests > > :100644 100644 8196fe2... cb987f6... M > common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java > :100644 100644 82289e8... 3a6205e... M > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java > :100644 100644 5d80dce... 7248d7c... M > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java > :000000 100644 0000000... 85f895d... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestConnectorHandling.java > :000000 100644 0000000... 6022bb3... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestDriverHandling.java > :000000 100644 0000000... 1dfacb5... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestJobHandling.java > :000000 100644 0000000... 54c598e... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestLinkHandling.java > :100644 100644 2da19bc... 4735240... M > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestStructure.java > :000000 100644 0000000... 9db7940... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestSubmissionHandling.java > > > Diffs > ----- > > > common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java > fb4e7af > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java > 82289e8 > > common-test/src/main/java/org/apache/sqoop/common/test/db/PostgreSQLProvider.java > d46e01d > repository/repository-postgresql/pom.xml 0ee9081 > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java > bdefd4c > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java > 4013d22 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestCase.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestUtils.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestConnectorHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestDriverHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestJobHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestLinkHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestStructure.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestSubmissionHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java > ae546f3 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestUtils.java > f393521 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestStructure.java > 6075de4 > > Diff: https://reviews.apache.org/r/29749/diff/ > > > Testing > ------- > > mvn clean test -pl repository/repository-postgresql > -Dsqoop.provider.class=org.apache.sqoop.common.test.db.PostgreSQLProvider > -Dsqoop.provider.postgresql.jdbc=jdbc:postgresql://.../test > -Dsqoop.provider.postgresql.username=test > -Dsqoop.provider.postgresql.password=test > > > Thanks, > > Abraham Elmahrek > >
