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


Ship it!




Hi Feró,

Thank you for fixing all the findings, it looks good now.

+1 for the separate doc JIRA

- Szabolcs Vasas


On Feb. 14, 2018, 9:45 a.m., Fero Szabo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65607/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2018, 9:45 a.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-2976
>     https://issues.apache.org/jira/browse/SQOOP-2976
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> **Summary:**
> Certain databases, such as SQL Server and Postgres are storing decimal values 
> padded with 0s, should the user insert them with less digits than the given 
> scale. 
> 
> Other databases however, such as Oracle and HSQLDB store these numbers 
> without trailing 0s. Then, when the JDBC driver returns these as BigDecimals, 
> they won't match the scale in the avro schema.
> 
> Take the following SQL commands for an example: 
> ```
> create table salary (id int, amount number (10,5));
> insert into salary (id, amount) values (1, 10.5);
> insert into salary (id, amount) values (2, 10.50);
> select * from salary;
> ```
> Records in an Oracle database:
> 1     10.5
> 2     10.5
> 
> Records in SQL Server (using decimal instead of number in the create 
> statement):
> 1     10.50000
> 2     10.50000
> 
> **Solition:**
> The fix is simply checking the scale of the returned BigDecimals against 
> what's in the avro schema and recreates the objects in case of a mismatch. 
> I've introduced a new property to enable this new feature, so existing 
> behavior is not affected. 
> 
> **Notes: **
> - trimmings cannot happen, as we generate our schema based on the database 
> columns. Therefore, the values passed to AvroUtil#toAvro will have less or 
> equal scale as the avro schema.
> - I will open a follow-up jira to document this change.
> 
> **Other notable changes:**
> - Introduced ArgumentArrayBuilder that reuses the existing Argument class and 
> introduces a useful builder pattern for creating commandline arguments for 
> tests.
> - Slightly modified BaseSqoopTest to fit my needs. *(However, further 
> refactoring would be required in this class to enable better reuse. For 
> example: the current implementation can't be used with SQL Server, because 
> one also needs to specify the schema besides the tablename in the create and 
> insert statements. There are also code duplications.)*
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/avro/AvroUtil.java 1aae8df2 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 7a19a62c 
>   src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java a5e5bf5a 
>   src/test/org/apache/sqoop/TestAvroImport.java 1172fc5d 
>   src/test/org/apache/sqoop/manager/hsqldb/TestHsqldbAvroPadding.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/metastore/TestMetastoreConfigurationParameters.java 
> 391dc336 
>   src/test/org/apache/sqoop/testutil/ArgumentArrayBuilder.java PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/ArgumentUtils.java 2f95e455 
>   src/test/org/apache/sqoop/testutil/AvroTestUtils.java PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 588f439c 
> 
> 
> Diff: https://reviews.apache.org/r/65607/diff/5/
> 
> 
> Testing
> -------
> 
> See the 3 new test classes (HSQLDB, Oracle, SQL Server).
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Reply via email to