-----------------------------------------------------------
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
>
>
