> On April 16, 2018, 2:28 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java > > Lines 131 (patched) > > <https://reviews.apache.org/r/66446/diff/3/?file=1998303#file1998303line131> > > > > Now that we have MySqlDatabaseAdapter shouldn't we move this method to > > that class?
As we discussed, I'd like to keep the scope of this change smaller, so we can do this refactor in another one. - Fero ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66446/#review201204 ----------------------------------------------------------- On April 17, 2018, 12:29 p.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66446/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 12:29 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/avro/AvroUtil.java caed90ea > 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/AvroTestUtils.java 75940bf1 > 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 > > > Diff: https://reviews.apache.org/r/66446/diff/4/ > > > Testing > ------- > > unit tests and 3rd party tests. > > > Thanks, > > Fero Szabo > >
