You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sqoop.apache.org by Nick White <nw...@palantir.com> on 2013/07/25 17:38:22 UTC

Review Request 12949: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

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

Review request for Sqoop.


Repository: sqoop-trunk


Description
-------

The patch adds a PostgresqlExportJob that replaces the OutputFormat (if needed) with a PostgresqlExportOutputFormat that inserts casts into the generated SQL statement (e.g. insert into mytable values (?, ?::inet, ?)). The patch also consolidates the various functions on ConnManager that return SQL type ints and type names into just one for each. This means the chunks of code in various parts of the codebase that select which of the three (former) methods to call can be replaced with a single call - and the call "routing" logic only appears in one place (ConnManager).


Diffs
-----

  src/java/org/apache/sqoop/hive/TableDefWriter.java c9962e9 
  src/java/org/apache/sqoop/manager/ConnManager.java c84c859 
  src/java/org/apache/sqoop/manager/MySQLManager.java e1d5a36 
  src/java/org/apache/sqoop/manager/OracleManager.java 686bc19 
  src/java/org/apache/sqoop/manager/PostgresqlManager.java bd882b9 
  src/java/org/apache/sqoop/manager/SqlManager.java 2a4992d 
  src/java/org/apache/sqoop/mapreduce/ExportOutputFormat.java c2e39b1 
  src/java/org/apache/sqoop/mapreduce/JdbcExportJob.java fee78e0 
  src/java/org/apache/sqoop/mapreduce/PostgresqlExportJob.java PRE-CREATION 
  src/java/org/apache/sqoop/mapreduce/PostgresqlExportOutputFormat.java PRE-CREATION 
  src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java a109b40 
  src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 806bace 
  src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41 
  src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java 0ac4599 
  src/test/com/cloudera/sqoop/manager/TestSqlManager.java 57855fa 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerManualTest.java ee576c9 

Diff: https://reviews.apache.org/r/12949/diff/


Testing
-------

I've added two cases to PostgresqlExportTest and tested them against a 9.3 database.


Thanks,

Nick White

Re: Review Request 12949: SQOOP-1149: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

Posted by Nick White <nw...@palantir.com>.

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

(Updated Aug. 1, 2013, 3:47 p.m.)

Review request for Sqoop.

Changes
-------

(removed unneeded cast)

Repository: sqoop-trunk

Description
-------

The patch adds a PostgresqlExportJob that replaces the OutputFormat (if needed) with a PostgresqlExportOutputFormat that inserts casts into the generated SQL statement (e.g. insert into mytable values (?, ?::inet, ?)). The patch also consolidates the various functions on ConnManager that return SQL type ints and type names into just one for each. This means the chunks of code in various parts of the codebase that select which of the three (former) methods to call can be replaced with a single call - and the call "routing" logic only appears in one place (ConnManager).

Diffs (updated)
-----

src/java/com/cloudera/sqoop/lib/JdbcWritableBridge.java 316547f
src/java/org/apache/sqoop/lib/JdbcWritableBridge.java afde585
src/java/org/apache/sqoop/manager/ConnManager.java f4b22f9
src/java/org/apache/sqoop/manager/PostgresqlManager.java bd882b9
src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java 0ac4599

Diff: https://reviews.apache.org/r/12949/diff/

Testing
-------

I've added two cases to PostgresqlExportTest and tested them against a 9.3 database.

Thanks,

Nick White

Re: Review Request 12949: SQOOP-1149: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

Posted by Nick White <nw...@palantir.com>.

(Updated Aug. 1, 2013, 3:43 p.m.)

Review request for Sqoop.

Changes
-------

I think I've come up with a nice way to abstract driver-specific Object types from the ClassWriter - see the attached (backwards-compatible) patch.

Repository: sqoop-trunk

Description
-------

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/12949/diff/

Testing
-------

I've added two cases to PostgresqlExportTest and tested them against a 9.3 database.

Thanks,

Nick White

Re: Review Request 12949: SQOOP-1149: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

Posted by Nick White <nw...@palantir.com>.

(Updated July 29, 2013, 1:37 a.m.)

Review request for Sqoop.

Changes
-------

Actually, I've been thinking about this a bit more, and have come up with the attached. Instead of re-writing the SQL INSERT statement to insert casts, it just uses the postgres driver's generic object class, org.postgresql.util.PGobject, in the generated code. The JdbcWritableBridge passes these PGobjects to and from a ResultSet's readObject / writeObject methods without needing to modifying them. The only downside to this approach is that ClassWriter uses the implementation-specific PGobject class. Can you think of a way around this? Maybe the ConnManager subclasses should know how to generate code for their database's object types?

Repository: sqoop-trunk

Description
-------

Diffs (updated)
-----

src/java/com/cloudera/sqoop/lib/JdbcWritableBridge.java 316547f
src/java/org/apache/sqoop/lib/JdbcWritableBridge.java afde585
src/java/org/apache/sqoop/manager/PostgresqlManager.java bd882b9
src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41
src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java 0ac4599

Diff: https://reviews.apache.org/r/12949/diff/

Testing
-------

I've added two cases to PostgresqlExportTest and tested them against a 9.3 database.

Thanks,

Nick White

Re: Review Request 12949: SQOOP-1149: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

Posted by Nick White <nw...@palantir.com>.

(Updated July 28, 2013, 6:05 p.m.)

Review request for Sqoop.

Changes
-------

I've revised the patch to maintain backwards compatibility. Maybe Sqoop 2.0 should hide how the types are gathered (e.g. from a table / query / stored procedure) from a database? The patch overrides toJavaType in PostgresqlManager as you suggest, but I still have to tweak the generated SQL when exporting as the postgres driver gives all custom types an SQL type of 1111 (OTHER). Postgres doesn't automatically convert Strings to custom types (e.g. http://stackoverflow.com/questions/3080470/jdbctemplate-and-inet-data-type); it needs explicit casts. Thanks -

Summary (updated)
-----------------

SQOOP-1149: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

Repository: sqoop-trunk

Description
-------

Diffs (updated)
-----

src/java/org/apache/sqoop/manager/ConnManager.java f4b22f9
src/java/org/apache/sqoop/manager/PostgresqlManager.java bd882b9
src/java/org/apache/sqoop/mapreduce/ExportOutputFormat.java c2e39b1
src/java/org/apache/sqoop/mapreduce/PostgresqlExportJob.java PRE-CREATION
src/java/org/apache/sqoop/mapreduce/PostgresqlExportOutputFormat.java PRE-CREATION
src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java 0ac4599

Diff: https://reviews.apache.org/r/12949/diff/

Testing
-------

I've added two cases to PostgresqlExportTest and tested them against a 9.3 database.

Thanks,

Nick White

Re: Review Request 12949: Support custom postgres types (e.g. inet for IP addresses) - which includes postgres enums.

Posted by Jarek Cecho <ja...@apache.org>.

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


Hi Nick,
thank you very much for working on this patch. It seems that submitted patch is in addition to adding support for additional data types for PostgreSQL doing quite huge refactoring of the ConnManager class. As that refactoring is not backward compatible, I'm afraid that it can't be accepted.

Adding support for custom types should be fairy straighforward, you need to overload the toJavaType(), toHiveType() and toAvroType() method. For examples, take a look on the Oracle connector:

https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/manager/OracleManager.java#L551

Jarcec

- Jarek Cecho


On July 25, 2013, 3:38 p.m., Nick White wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12949/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 3:38 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> The patch adds a PostgresqlExportJob that replaces the OutputFormat (if needed) with a PostgresqlExportOutputFormat that inserts casts into the generated SQL statement (e.g. insert into mytable values (?, ?::inet, ?)). The patch also consolidates the various functions on ConnManager that return SQL type ints and type names into just one for each. This means the chunks of code in various parts of the codebase that select which of the three (former) methods to call can be replaced with a single call - and the call "routing" logic only appears in one place (ConnManager).
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/hive/TableDefWriter.java c9962e9 
>   src/java/org/apache/sqoop/manager/ConnManager.java c84c859 
>   src/java/org/apache/sqoop/manager/MySQLManager.java e1d5a36 
>   src/java/org/apache/sqoop/manager/OracleManager.java 686bc19 
>   src/java/org/apache/sqoop/manager/PostgresqlManager.java bd882b9 
>   src/java/org/apache/sqoop/manager/SqlManager.java 2a4992d 
>   src/java/org/apache/sqoop/mapreduce/ExportOutputFormat.java c2e39b1 
>   src/java/org/apache/sqoop/mapreduce/JdbcExportJob.java fee78e0 
>   src/java/org/apache/sqoop/mapreduce/PostgresqlExportJob.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/PostgresqlExportOutputFormat.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java a109b40 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 806bace 
>   src/java/org/apache/sqoop/orm/ClassWriter.java 1bd2a41 
>   src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java 0ac4599 
>   src/test/com/cloudera/sqoop/manager/TestSqlManager.java 57855fa 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerManualTest.java ee576c9 
> 
> Diff: https://reviews.apache.org/r/12949/diff/
> 
> 
> Testing
> -------
> 
> I've added two cases to PostgresqlExportTest and tested them against a 9.3 database.
> 
> 
> Thanks,
> 
> Nick White
> 
>