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



Hi Feró,

Thank you for fixing my findings, I think the code generally looks now I have 
left some minor comments.


src/java/org/apache/sqoop/manager/oracle/OracleUtils.java
Lines 86 (patched)
<https://reviews.apache.org/r/66446/#comment282336>

    This method looks really similar to 
org.apache.sqoop.manager.ConnManager#toAvroLogicalType(int, java.lang.Integer, 
java.lang.Integer). Can we somehow reorganize the code to contain less 
duplications?



src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java
Lines 54 (patched)
<https://reviews.apache.org/r/66446/#comment282333>

    I think it would be good to add some comments to this test case to clarify 
what exactly it wants to test.



src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java
Lines 68 (patched)
<https://reviews.apache.org/r/66446/#comment282318>

    typo: SUCCEED



src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java
Lines 71 (patched)
<https://reviews.apache.org/r/66446/#comment282332>

    It is a small thing but I think it would improve the readability of the 
test results if you would override the toString() method of the adapters and 
the configuration classes to return the simple name of the class, currently the 
parameter name gets really long.



src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java
Lines 89 (patched)
<https://reviews.apache.org/r/66446/#comment282334>

    The variable naming is confusing here.



src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
Lines 23 (patched)
<https://reviews.apache.org/r/66446/#comment282335>

    Is there a reason this abstract class is not an interface?
    
    Is this class hierarchy meant to be used in other test cases too or only in 
AvroImportForNumericTypesTest? If latter then I think it should be moved to the 
same package as AvroImportForNumericTypesTest and the name should be less 
generic.



src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java
Lines 131 (patched)
<https://reviews.apache.org/r/66446/#comment282324>

    Now that we have MySqlDatabaseAdapter shouldn't we move this method to that 
class?



src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java
Lines 24 (patched)
<https://reviews.apache.org/r/66446/#comment282325>

    Unused import.



src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java
Lines 26 (patched)
<https://reviews.apache.org/r/66446/#comment282326>

    Unused import.



src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java
Lines 27 (patched)
<https://reviews.apache.org/r/66446/#comment282327>

    Unused import.



src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java
Lines 31 (patched)
<https://reviews.apache.org/r/66446/#comment282328>

    Unused import.



src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java
Lines 33 (patched)
<https://reviews.apache.org/r/66446/#comment282329>

    Unused import.



src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java
Lines 34 (patched)
<https://reviews.apache.org/r/66446/#comment282330>

    Unused import.



src/test/org/apache/sqoop/testutil/adapter/DatabaseAdapter.java
Lines 28 (patched)
<https://reviews.apache.org/r/66446/#comment282319>

    nit: there are 2 spaces before the method name.



src/test/org/apache/sqoop/testutil/adapter/DatabaseAdapter.java
Lines 30 (patched)
<https://reviews.apache.org/r/66446/#comment282320>

    nit: public is redundant for interface methods.



src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java
Lines 27 (patched)
<https://reviews.apache.org/r/66446/#comment282321>

    Formatting: 2 spaces before DatabaseAdapter but no space before {



src/test/org/apache/sqoop/testutil/adapter/OracleDatabaseAdapter.java
Lines 28 (patched)
<https://reviews.apache.org/r/66446/#comment282322>

    Formatting: 2 spaces before DatabaseAdapter



src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java
Line 42 (original), 42 (patched)
<https://reviews.apache.org/r/66446/#comment282323>

    This reformatting does not seem to be related to this patch, can you please 
remove it?


- Szabolcs Vasas


On April 13, 2018, 3:45 p.m., Fero Szabo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66446/
> -----------------------------------------------------------
> 
> (Updated April 13, 2018, 3:45 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> 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
> -----
> 
>   src/docs/user/import.txt e91a5a84 
>   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/3/
> 
> 
> Testing
> -------
> 
> unit tests and 3rd party tests.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Reply via email to