----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65607/#review197274 -----------------------------------------------------------
Hi Fero, Thank you very much for this improvement, I really appreciate that you added this many new test cases! Regarding the flag naming I would use something like avro.decimal_scale_matching.enable as harmonization is too generic for me. I would like to see other's opinion about this too, also please be aware that in case of changing from "padding" you should change all the related method names too. Unfortunately the documentation is not that detailed regarding Avro file format but maybe you could add a subsection under "File Formats" section in src/docs/user/import.txt about this flag and it's behavior. I ran unit and third party tests with your patch successfuly. Although, I have couple of comments regarding your change (which is quite big), please find them below. Thank you, Bogi src/java/org/apache/sqoop/config/ConfigurationConstants.java Lines 109 (patched) <https://reviews.apache.org/r/65607/#comment277398> Please update this comment too (I assume it is from copy+paste). src/test/org/apache/sqoop/manager/hsqldb/TestHsqldbAvroPadding.java Lines 52-53 (patched) <https://reviews.apache.org/r/65607/#comment277400> Could you please remove the whitespace between String and []? src/test/org/apache/sqoop/manager/hsqldb/TestHsqldbAvroPadding.java Lines 70 (patched) <https://reviews.apache.org/r/65607/#comment277401> Could we have a more descriptive error message at the negative case maybe? src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java Lines 98-101 (patched) <https://reviews.apache.org/r/65607/#comment277527> Do we really need this here? src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java Lines 107-108 (patched) <https://reviews.apache.org/r/65607/#comment277402> Could you please clean up comments like these? src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java Lines 131-134 (patched) <https://reviews.apache.org/r/65607/#comment277404> Where does this javadoc come from? src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java Lines 144-147 (patched) <https://reviews.apache.org/r/65607/#comment277405> Where does this javadoc come from? src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java Lines 70 (patched) <https://reviews.apache.org/r/65607/#comment277534> Do we really need to reimplement this method? Couldn't you use the one in BaseSqoopTestCase instead? src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java Lines 71 (patched) <https://reviews.apache.org/r/65607/#comment277406> Why stmt3? src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java Lines 109 (patched) <https://reviews.apache.org/r/65607/#comment277535> Same question here. src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java Lines 45-60 (patched) <https://reviews.apache.org/r/65607/#comment277536> This has already been documented in the COMPILING.txt so I don't think it is necessary here. Please take a look. - Boglarka Egyed On Feb. 12, 2018, 2:31 p.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65607/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2018, 2:31 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. > > **Concerns: ** > - trimmings can happen silently, should we rather raise an exception? > Enabling trimming adds a new feature, but it also adds the possibility > silently lose scale while import. The latter could be mitigated by a thorough > documentation. > - The flags current name () doesn't really match the behavior, should I > change it to something else? (avro.decimal_scale_harmonization.enable) > - How / where to document this new flag? > > > **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/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/testutil/ArgumentArrayBuilder.java PRE-CREATION > 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/2/ > > > Testing > ------- > > See the 3 new test classes (HSQLDB, Oracle, SQL Server). > > > Thanks, > > Fero Szabo > >
