Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

2018-11-13 Thread Chris Teoh 

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

(Updated Nov. 14, 2018, 10:24 a.m.)


Review request for Sqoop.


Changes
---

Corrected logic, and fixed unit tests after refactor.


Bugs: SQOOP-3237
https://issues.apache.org/jira/browse/SQOOP-3237


Repository: sqoop-trunk


Description
---

Added --ftpcmds command to allow comma separated list of FTP commands to send.


Diffs (updated)
-

  src/docs/user/import-mainframe.txt 3ecfb7e4 
  src/java/org/apache/sqoop/SqoopOptions.java f06872f9 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
9842daa6 
  src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java 
90dc2ddd 
  src/java/org/apache/sqoop/tool/MainframeImportTool.java fbc8c3db 
  src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java e7c48a6b 
  src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 00e57bd0 
  src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 0714bdcf 


Diff: https://reviews.apache.org/r/62523/diff/9/

Changes: https://reviews.apache.org/r/62523/diff/8-9/


Testing
---

Unit tests.


File Attachments


SQOOP-3237-1.patch
  
https://reviews.apache.org/media/uploaded/files/2017/09/26/56041556-e355-4372-83ab-1bcc01680201__SQOOP-3237-1.patch


Thanks,

Chris Teoh

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

2018-11-13 Thread Szabolcs Vasas 

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


Ship it!




Hi Feró,

Thank you for fixing all the findings, I have ran the unit and third party 
tests with the latest patch and all of them passed.
Ship it!

- Szabolcs Vasas


On Nov. 12, 2018, 4:33 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69060/
> ---
> 
> (Updated Nov. 12, 2018, 4:33 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/ParquetImportMapper.java 62334f8ab 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
>  e82154309 
>   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/4/
> 
> 
> Testing
> ---
> 
> 3rd party tests and unit tests, both gradle and ant
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

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

2018-11-13 Thread Boglarka Egyed 


> On Nov. 9, 2018, 2:26 p.m., Boglarka Egyed wrote:
> > 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).
> 
> Fero Szabo wrote:
> Hi Bogi,
> 
> Thanks for the review!
> 
> There is a tiny difference: to enable logical types in parquet, there is 
> new flag (sqoop.parquet.logical_types.decimal.enable), i.e. only used in the 
> parquet tests. 
> 
> I'd keep this code as is, as deduplication might lead to spaghetti code 
> here (since these are different features after all).
> 
> Even though this is a bit of a compromise, I'd like to drop this issue if 
> that's OK with you (?)

Thanks for pointing this out, Fero. I think you are right about creating 
spaghetti code here, please feel free to drop my previous comment. Also, the 
tests are now easy-to-read which is good as it is.


- Boglarka


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


On Nov. 12, 2018, 4:33 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69060/
> ---
> 
> (Updated Nov. 12, 2018, 4:33 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/ParquetImportMapper.java 62334f8ab 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
>  e82154309 
>   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/4/
> 
> 
> Testing
> ---
> 
> 3rd party tests and unit tests, both gradle and ant
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>