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

(Updated April 13, 2018, 3:02 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
-------

Hi Szabi,

Thank you for your insights, I believe this is a much better design now!


Bugs: SQOOP-2567
    https://issues.apache.org/jira/browse/SQOOP-2567


Repository: sqoop-trunk


Description
-------

This fix allows the user to specify default precision and scale for avro 
schemas. The default values are then used to override "invalid" values, (when 
the database returns 0s as precision) and in case of oracle, the -127 scale 
value. 

**Key points**
- The implementation takes place in the ConnManager#toAvroLogicalType function 
and the overriding funcitons in OraOopConnManager and OracleManager
- Testing is covered very thoroughly by the TestAvroImportForNumericTypes class 
and multiple configurations are used to cover MySQL, Oracle, Postgres and MS 
SQL.

**Implementation specific concerns**
- The edge cases aren't well documented. These tests aim to cover the 
NUMBER/NUMERIC and DECIMAL types with or without specified scale and precision 
thoroughly. Are there any missed testcases?
- The new parameters act as overrides only for PSQL and Oracle databases, 
because we the other databases translate the missing precision to valid values. 
Even though this is true, I've added testcases for MS SQL and MySQL.

- In case of Oracle 
The databae returns if user doesn't specify the default scale and the db return 
-127, we adjust the precision by that much.
Should we throw an exception instead?

- The default precision has to be specified. If it's not there and the database 
returns 0 we throw an exception. 
- Instead, if the default precision and scale aren't there, we could just use 
the maximum possible value i.e. 38 + 127 = 165 as precision and 127 as scale, 
that would fit everything in a very inefficient manner, mostly containing 0s. 
(This also opens up the question whether there is an efficient way to store 
numbers with many 0s in avro.)

**Testing specific concerns**
- The ImportJobTestConfiguration#dropTableIfExists method is not really a test 
configuration related method, however at the time of development, it made sense 
to have it there. This might be better off in another place, such as 
BaseSqoopTest (though I'm unsure how that implementation would look like.)
- The SqlUtil class was created solely to provide a place for the 
executeStatement method. This might also be better off in another class, such 
as BaseSqoopTest.


Diffs (updated)
-----

  src/java/org/apache/sqoop/config/ConfigurationConstants.java 2197025b 
  src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a6998 
  src/java/org/apache/sqoop/manager/ConnManager.java d88b59bd 
  src/java/org/apache/sqoop/manager/OracleManager.java 929b5061 
  src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f 
  src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 09207bb4 
  src/java/org/apache/sqoop/manager/oracle/OracleUtils.java aa56e708 
  src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MSSQLServerImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MySQLImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfigurationForNumber.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationForNumeric.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 PRE-CREATION 
  src/test/org/apache/sqoop/manager/mysql/MySQLLobAvroImportTest.java a6121c9a 
  src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java 75ecc357 
  src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
f217f0bc 
  src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 6d752aa4 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlImportTest.java 
846228a1 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 2220b7d5 
  
src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java 
27dc0cd7 
  src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java a5f85a06 
  src/test/org/apache/sqoop/testutil/SqlUtil.java PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/DatabaseAdapter.java PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/OracleDatabaseAdapter.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/PostgresDatabaseAdapter.java 
PRE-CREATION 
  src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java bdac437f 


Diff: https://reviews.apache.org/r/66446/diff/2/

Changes: https://reviews.apache.org/r/66446/diff/1-2/


Testing
-------

unit tests and 3rd party tests.


Thanks,

Fero Szabo

Reply via email to