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



Hi Fero,

Thank you for this nice addition!

I have started to look into your change, you can find some minor findings 
below, but I ran into some failing unit tests in TestHsqldbAvroPadding, e.g.:

Testcase: testAvroImportWithPadding took 0.853 sec
        FAILED
Could not insert into table: java.sql.SQLException: Unexpected token: AARON in 
statement [INSERT INTO "IMPORT_TABLE_1"("ID", "NAME", "SALARY", "DEPT") 
VALUES('1', ''Aaron'', '1000000.05', ''engineering'')]
        at org.hsqldb.jdbc.Util.throwError(Unknown Source)
        at org.hsqldb.jdbc.jdbcPreparedStatement.<init>(Unknown Source)
        at org.hsqldb.jdbc.jdbcConnection.prepareStatement(Unknown Source)
        at 
org.apache.sqoop.testutil.BaseSqoopTestCase.insertIntoTable(BaseSqoopTestCase.java:466)
        at 
org.apache.sqoop.manager.hsqldb.TestHsqldbAvroPadding.createTestTable(TestHsqldbAvroPadding.java:54)
        at 
org.apache.sqoop.manager.hsqldb.TestHsqldbAvroPadding.setUp(TestHsqldbAvroPadding.java:46)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
        at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
        at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
        at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
        at 
org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:239)
        at org.junit.rules.RunRules.evaluate(RunRules.java:20)
        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
        at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
        at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
        at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:38)
        at 
org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:535)
        at 
org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:1182)
        at 
org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:1033)

junit.framework.AssertionFailedError: Could not insert into table: 
java.sql.SQLException: Unexpected token: AARON in statement [INSERT INTO 
"IMPORT_TABLE_1"("ID", "NAME", "SALARY", "DEPT") VALUES('1', ''Aaron'', 
'1000000.05', ''engineering'')]
        at org.hsqldb.jdbc.Util.throwError(Unknown Source)
        at org.hsqldb.jdbc.jdbcPreparedStatement.<init>(Unknown Source)
        at org.hsqldb.jdbc.jdbcConnection.prepareStatement(Unknown Source)
        at 
org.apache.sqoop.testutil.BaseSqoopTestCase.insertIntoTable(BaseSqoopTestCase.java:466)
        at 
org.apache.sqoop.manager.hsqldb.TestHsqldbAvroPadding.createTestTable(TestHsqldbAvroPadding.java:54)
        at 
org.apache.sqoop.manager.hsqldb.TestHsqldbAvroPadding.setUp(TestHsqldbAvroPadding.java:46)

        at 
org.apache.sqoop.testutil.BaseSqoopTestCase.insertIntoTable(BaseSqoopTestCase.java:471)
        at 
org.apache.sqoop.manager.hsqldb.TestHsqldbAvroPadding.createTestTable(TestHsqldbAvroPadding.java:54)
        at 
org.apache.sqoop.manager.hsqldb.TestHsqldbAvroPadding.setUp(TestHsqldbAvroPadding.java:46)

Could you please take a look at this?

Thanks,
Bogi


src/java/org/apache/sqoop/manager/ConnManager.java
Line 230 (original), 234 (patched)
<https://reviews.apache.org/r/66446/#comment282315>

    Parameters precision and scale are missing from this javadoc.



src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java
Lines 66-69 (patched)
<https://reviews.apache.org/r/66446/#comment282317>

    Why aren't these fields final?



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

    typo: SUCCED


- Boglarka Egyed


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