Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

2018-11-09 Thread Boglarka Egyed 

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



Hi Fero,

Thanks for this improvement!

I have left a couple of comments related to testing, please find them below.

Thanks,
Bogi


src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java
Lines 220 (patched)


I think these tests could be parameterized as they are doing the same but 
with different file formats (Avro and Parquet).



src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java
Lines 299 (patched)


Why aren't you assert the result as a list?



src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java
Lines 301-305 (patched)


With this logic now you don't test if the size of the expected result and 
the output are the same.


- Boglarka Egyed


On Nov. 8, 2018, 3:34 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69060/
> ---
> 
> (Updated Nov. 8, 2018, 3:34 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3382
> https://issues.apache.org/jira/browse/SQOOP-3382
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This patch is about adding support for fixed point decimal types in parquet 
> import.
> 
> The implementation is simple after the fact that parquet was upgraded to 
> 1.9.0 in SQOOP-3381: we just need to register the GenericDataSupplier with 
> AvroParquetOutputFormat.
> 
> For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
> under the hood to write parquet.
> 
> I also moved around and renamed the classes involved in this change so their 
> name and package reflect their purpose.
> 
> ** Note: A key design decision can be seen in the ImportJobTestConfiguration 
> interface **
> - I decided to create a new function to get the expected results for each 
> file format, since we seldom add new fileformats. 
> - However this also enforces future configurations to always define their 
> expected result for every file forma or throw a NotImplementedException 
> should they lack the support for one.
> - The alternative for this is to define the fileLayout as an input parameter 
> instead. This would allow for better extendability.
> _Please share your thoughts on this!_
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250e 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c069888 
>   src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8ab 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9cd 
>   src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 
> 14de910b9 
>   src/test/org/apache/sqoop/importjob/SplitByImportTest.java 7977c0b0f 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
>  182d2967f 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
>  e9bf9912a 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
>  b7bad08c0 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
>  465e61f4b 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
>  66715c171 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
>  ec4db41bd 
>   
> src/test/org/apache/sqoop/importjob/configuration/AvroTestConfiguration.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
>  f137b56b7 
>   
> src/test/org/apache/sqoop/importjob/configuration/ParquetTestConfiguration.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/util/ParquetReader.java 908ce566f 
> 
> 
> Diff: https://reviews.apache.org/r/69060/diff/3/
> 
> 
> Testing
> ---
> 
> 3rd party tests and unit tests, both gradle and ant
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

[jira] [Resolved] (SQOOP-3403) Sqoop2: Add Fero Szabo to committer list in our pom file

2018-11-09 Thread Fero Szabo (JIRA) 


 [ 
https://issues.apache.org/jira/browse/SQOOP-3403?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Fero Szabo resolved SQOOP-3403.
---
Resolution: Fixed

> Sqoop2: Add Fero Szabo to committer list in our pom file
> 
>
> Key: SQOOP-3403
> URL: https://issues.apache.org/jira/browse/SQOOP-3403
> Project: Sqoop
>  Issue Type: Task
>Affects Versions: 1.99.8
>Reporter: Boglarka Egyed
>Assignee: Fero Szabo
>Priority: Major
>
> Now that [~fero] is committer we should update our committer list in the root 
> pom.xml file:



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (SQOOP-3403) Sqoop2: Add Fero Szabo to committer list in our pom file

2018-11-09 Thread Hudson (JIRA) 


[ 
https://issues.apache.org/jira/browse/SQOOP-3403?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16681280#comment-16681280
 ] 

Hudson commented on SQOOP-3403:
---

FAILURE: Integrated in Jenkins build Sqoop2 #1056 (See 
[https://builds.apache.org/job/Sqoop2/1056/])
SQOOP-3403: Sqoop2: Add Fero Szabo to committer list in our pom file (fero: 
[https://git-wip-us.apache.org/repos/asf?p=sqoop.git=commit=8382ffb9ce74e5a7ac37dac010eeebf034a6d5fb])
* (edit) pom.xml


> Sqoop2: Add Fero Szabo to committer list in our pom file
> 
>
> Key: SQOOP-3403
> URL: https://issues.apache.org/jira/browse/SQOOP-3403
> Project: Sqoop
>  Issue Type: Task
>Affects Versions: 1.99.8
>Reporter: Boglarka Egyed
>Assignee: Fero Szabo
>Priority: Major
>
> Now that [~fero] is committer we should update our committer list in the root 
> pom.xml file:



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (SQOOP-3403) Sqoop2: Add Fero Szabo to committer list in our pom file

2018-11-09 Thread ASF subversion and git services (JIRA) 


[ 
https://issues.apache.org/jira/browse/SQOOP-3403?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16681278#comment-16681278
 ] 

ASF subversion and git services commented on SQOOP-3403:


Commit 8382ffb9ce74e5a7ac37dac010eeebf034a6d5fb in sqoop's branch 
refs/heads/sqoop2 from [~fero]
[ https://git-wip-us.apache.org/repos/asf?p=sqoop.git;h=8382ffb ]

SQOOP-3403: Sqoop2: Add Fero Szabo to committer list in our pom file

(Fero Szabo)


> Sqoop2: Add Fero Szabo to committer list in our pom file
> 
>
> Key: SQOOP-3403
> URL: https://issues.apache.org/jira/browse/SQOOP-3403
> Project: Sqoop
>  Issue Type: Task
>Affects Versions: 1.99.8
>Reporter: Boglarka Egyed
>Assignee: Fero Szabo
>Priority: Major
>
> Now that [~fero] is committer we should update our committer list in the root 
> pom.xml file:



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Updated] (SQOOP-3403) Sqoop2: Add Fero Szabo to committer list in our pom file

2018-11-09 Thread Boglarka Egyed (JIRA) 


 [ 
https://issues.apache.org/jira/browse/SQOOP-3403?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Boglarka Egyed updated SQOOP-3403:
--
Description: Now that [~fero] is committer we should update our committer 
list in the root pom.xml file:

> Sqoop2: Add Fero Szabo to committer list in our pom file
> 
>
> Key: SQOOP-3403
> URL: https://issues.apache.org/jira/browse/SQOOP-3403
> Project: Sqoop
>  Issue Type: Task
>Affects Versions: 1.99.8
>Reporter: Boglarka Egyed
>Assignee: Fero Szabo
>Priority: Major
>
> Now that [~fero] is committer we should update our committer list in the root 
> pom.xml file:



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (SQOOP-3403) Sqoop2: Add Fero Szabo to committer list in our pom file

2018-11-09 Thread Boglarka Egyed (JIRA) 
Boglarka Egyed created SQOOP-3403:
-

 Summary: Sqoop2: Add Fero Szabo to committer list in our pom file
 Key: SQOOP-3403
 URL: https://issues.apache.org/jira/browse/SQOOP-3403
 Project: Sqoop
  Issue Type: Task
Affects Versions: 1.99.8
Reporter: Boglarka Egyed
Assignee: Fero Szabo






--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[ANNOUNCE] New Sqoop committer: Fero Szabo

2018-11-09 Thread Boglarka Egyed 
On behalf of the Apache Sqoop PMC, I am excited to welcome Fero Szabo
as a new committer to Apache Sqoop. Please join me in congratulating him
for this accomplishment!

Fero has a decent contribution to the code[1] containing new features,
bugfixes
and documentation updates and he is one of the most active
reviewers[2] on the project. He constantly helps others on the mailing
lists and
the Jira, as a part of this effort he is also trying to resurrect old
patches
that could be useful but were abandoned.

We look forward to Fero's continued contributions!

1: https://s.apache.org/Im35
2: https://reviews.apache.org/users/fero/reviews/