sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boglarka Egyed <b...@apache.org>
Subject Re: Review Request 66446: SQOOP-2567 SQOOP import for Oracle fails with invalid precision/scale for decimal
Date Mon, 16 Apr 2018 14:39:51 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message