----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65607/#review197413 -----------------------------------------------------------
Hi Feró, Thank you for fixing the findings so quickly! I have found a few more small things, please take a look. src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java Lines 152 (patched) <https://reviews.apache.org/r/65607/#comment277572> It seems this method is not used, do we need it here? src/test/org/apache/sqoop/testutil/AvroTestUtils.java Lines 30 (patched) <https://reviews.apache.org/r/65607/#comment277573> Unused import. src/test/org/apache/sqoop/testutil/AvroTestUtils.java Lines 31 (patched) <https://reviews.apache.org/r/65607/#comment277574> Unused import. src/test/org/apache/sqoop/testutil/AvroTestUtils.java Lines 33 (patched) <https://reviews.apache.org/r/65607/#comment277575> Unused import. - Szabolcs Vasas On Feb. 13, 2018, 4:13 p.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65607/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2018, 4:13 p.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/MSSQLTestUtils.java 2220b7d5 > > 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/3/ > > > Testing > ------- > > See the 3 new test classes (HSQLDB, Oracle, SQL Server). > > > Thanks, > > Fero Szabo > >
