> On Feb. 13, 2018, 11:39 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/avro/AvroUtil.java
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/65607/diff/2/?file=1957645#file1957645line88>
> >
> >     I think this if statement is redundant since if schemaContainingScale 
> > is null then the method will return null anyway.

Nope, this is there because we are looking for the first non-null value 
returned by getDecimalSchema. Here is an example schema where this comes into 
play:
{"type" : "union", "value" : "{\"type\" : \"null\"}, {\"type\" : \"null\", 
\"precision\" : 10, \"scale\" : 5}"}

If we would remove the if statement, then getDecimalSchema would return null.

Maybe, I should have added 
```
else {
  continue;
}
```
for verbosity?


> On Feb. 13, 2018, 11:39 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java
> > Lines 101 (patched)
> > <https://reviews.apache.org/r/65607/diff/2/?file=1957649#file1957649line101>
> >
> >     Do we really need this file? It seems we do not use dates in this test 
> > case.

Nope, removed.


> On Feb. 13, 2018, 11:39 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java
> > Lines 93 (patched)
> > <https://reviews.apache.org/r/65607/diff/2/?file=1957651#file1957651line93>
> >
> >     A similar logic is already present in 
> > org.apache.sqoop.testutil.BaseSqoopTestCase can we somehow reuse that logic?
> >     For example it already has a ConnManager field which can be initialized 
> > in org.apache.sqoop.testutil.BaseSqoopTestCase#setUp

Thanks for pointing this one out. In the end I figured out how to reuse the 
table creation and insertion logic from BaseSqoopTestCase as well.


> On Feb. 13, 2018, 11:39 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/testutil/ArgumentArrayBuilder.java
> > Lines 102 (patched)
> > <https://reviews.apache.org/r/65607/diff/2/?file=1957652#file1957652line102>
> >
> >     I think this logic handling the tool options could be also moved to 
> > ArgumentUtils since it already contains similar stuff.
> >     After that this method could be simplified, the notEmpty checks could 
> > also be removed.
> >     
> >     We could also think about moving all the logic from ArgumentUtils to 
> > the builder since it would be a better practice to use the builder instead 
> > of the static methods.

I think the latter makes more sense, so I've moved the functions from 
ArgumentUtils to the builder and also replaced every call of ArgumentUtil's 
functions to use the new builder.


- Fero


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


On Feb. 12, 2018, 2:31 p.m., Fero Szabo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65607/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2018, 2:31 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-2976
>     https://issues.apache.org/jira/browse/SQOOP-2976
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> **Summary:**
> Certain databases, such as SQL Server and Postgres are storing decimal values 
> padded with 0s, should the user insert them with less digits than the given 
> scale. 
> 
> Other databases however, such as Oracle and HSQLDB store these numbers 
> without trailing 0s. Then, when the JDBC driver returns these as BigDecimals, 
> they won't match the scale in the avro schema.
> 
> Take the following SQL commands for an example: 
> ```
> create table salary (id int, amount number (10,5));
> insert into salary (id, amount) values (1, 10.5);
> insert into salary (id, amount) values (2, 10.50);
> select * from salary;
> ```
> Records in an Oracle database:
> 1     10.5
> 2     10.5
> 
> Records in SQL Server (using decimal instead of number in the create 
> statement):
> 1     10.50000
> 2     10.50000
> 
> **Solition:**
> The fix is simply checking the scale of the returned BigDecimals against 
> what's in the avro schema and recreates the objects in case of a mismatch. 
> I've introduced a new property to enable this new feature, so existing 
> behavior is not affected. 
> 
> **Concerns: **
> - trimmings can happen silently, should we rather raise an exception? 
> Enabling trimming adds a new feature, but it also adds the possibility 
> silently lose scale while import. The latter could be mitigated by a thorough 
> documentation.
> - The flags current name () doesn't really match the behavior, should I 
> change it to something else? (avro.decimal_scale_harmonization.enable)
> - How / where to document this new flag?
> 
> 
> **Other notable changes:**
> - Introduced ArgumentArrayBuilder that reuses the existing Argument class and 
> introduces a useful builder pattern for creating commandline arguments for 
> tests.
> - Slightly modified BaseSqoopTest to fit my needs. *(However, further 
> refactoring would be required in this class to enable better reuse. For 
> example: the current implementation can't be used with SQL Server, because 
> one also needs to specify the schema besides the tablename in the create and 
> insert statements. There are also code duplications.)*
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/avro/AvroUtil.java 1aae8df2 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 7a19a62c 
>   src/java/org/apache/sqoop/mapreduce/AvroImportMapper.java a5e5bf5a 
>   src/test/org/apache/sqoop/manager/hsqldb/TestHsqldbAvroPadding.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 2220b7d5 
>   
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/ArgumentArrayBuilder.java PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/AvroTestUtils.java PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 588f439c 
> 
> 
> Diff: https://reviews.apache.org/r/65607/diff/2/
> 
> 
> Testing
> -------
> 
> See the 3 new test classes (HSQLDB, Oracle, SQL Server).
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Reply via email to