----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66446/#review200998 -----------------------------------------------------------
Hi Feró, Thank you for submitting this patch, please find my findings inline. src/java/org/apache/sqoop/config/ConfigurationHelper.java Lines 255 (patched) <https://reviews.apache.org/r/66446/#comment281928> This variable can have a less specific name. src/java/org/apache/sqoop/manager/oracle/OracleUtils.java Lines 21 (patched) <https://reviews.apache.org/r/66446/#comment281930> Please remove unused imports. src/test/org/apache/sqoop/TestAvroImportForNumericTypes.java Lines 59 (patched) <https://reviews.apache.org/r/66446/#comment281934> Nice solution to parameterize the test cases instead of introducing inheritance! I have a couple of suggestions here: - ImportJobTestConfiguration contains methods which could be reused across different test cases (e.g. dropTableIfExists) and methods which are specific to TestAvroImportForNumericTypes. I think these should be split into seprate hierarchies and packages. - I would also move the failWithoutPadding and failWithoutDefaults boolean values from the configuration classes and provide them as a separate parameter to the TestAvroImportForNumericTypes. This would improve the readability of the test since one would not have to navigate to another classes to determine if a test case should succeed or fail. - If a test case succeeds with a specific org.apache.sqoop.testutil.configuration.ImportJobTestConfiguration#getTypes array that basically means that all of the enumerated data types work. However if a test case fails with such an array it means that Sqoop does not work with at least one of these types but it is not visible which one. I think it would be great if we could somehow separate these data types into separate configurations maybe. src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java Lines 26 (patched) <https://reviews.apache.org/r/66446/#comment281929> Unused import. - Szabolcs Vasas On April 12, 2018, 10:06 a.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66446/ > ----------------------------------------------------------- > > (Updated April 12, 2018, 10:06 a.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/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/TestAvroImportForNumericTypes.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/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/testutil/SqlUtil.java PRE-CREATION > > src/test/org/apache/sqoop/testutil/configuration/ImportJobTestConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/testutil/configuration/MSSQLServerImportJobTestConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/testutil/configuration/MySQLImportJobTestConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/testutil/configuration/OracleImportJobTestConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/testutil/configuration/PostgresqlImportJobTestConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/testutil/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66446/diff/1/ > > > Testing > ------- > > unit tests and 3rd party tests. > > > Thanks, > > Fero Szabo > >
