Re: Review Request 68541: SQOOP-3104: Create test categories instead of test suites and naming conventions

2018-11-12 Thread Nguyen Truong 

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

(Updated Nov. 13, 2018, 6:18 a.m.)


Review request for Sqoop.


Changes
---

Hi all,

Thank you for your patience.

Besides categories, I also changed a line in 
org.apache.sqoop.hbase.HBaseTestCase#setUp according to Szabolc's suggestion as 
it caused another test interdependency.

I have tested the patch with ./gradlew test.

Thank you,
Natalie


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


Repository: sqoop-trunk


Description
---

We are currently unsing test naming conventions to differentiate between 
ManualTests, Unit tests and 3rd party tests. Instead of that, I implemented 
junit categories which will allow us to have more categories in the future. 
This would also remove the reliance on the test class name.

Test categories skeleton:
  SqoopTest _ UnitTest
  |__ IntegrationTest
  |__ ManualTest

  ThirdPartyTest _ CubridTest
   |__ Db2Test
   |__ MainFrameTest
   |__ MysqlTest
   |__ NetezzaTest
   |__ OracleTest
   |__ PostgresqlTest
   |__ SqlServerTest

  KerberizedTest

Categories explanation:
* SqoopTest: Group of the big categories, including:
- UnitTest: It tests one class only with its dependencies mocked or if 
the dependency
is lightweight we can keep it. It must not start a minicluster or an 
hsqldb database.
It does not need JCDB drivers.
- IntegrationTest: It usually tests a whole scenario. It may start up 
miniclusters,
hsqldb and connect to external resources like RDBMSs.
- ManualTest: This should be a deprecated category which should not be 
used in the future.
It only exists to mark the currently existing manual tests.
* ThirdPartyTest: An orthogonal hierarchy for tests that need a JDBC driver 
and/or a docker
container/external RDBMS instance to run. Subcategories express what kind 
of external
resource the test needs. E.g: OracleTest needs an Oracle RDBMS and Oracle 
driver on the classpath
* KerberizedTest: Test that needs Kerberos, which needs to be run on a 
separate JVM.

Opinions are very welcomed. Thanks!


Diffs (updated)
-

  build.gradle 2014b5cf5 
  src/test/org/apache/sqoop/TestConnFactory.java fb6c94059 
  src/test/org/apache/sqoop/TestIncrementalImport.java 29c477954 
  src/test/org/apache/sqoop/TestSqoopOptions.java e55682edf 
  src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java 631eeff5e 
  src/test/org/apache/sqoop/authentication/TestKerberosAuthenticator.java 
f5700ce65 
  src/test/org/apache/sqoop/db/TestDriverManagerJdbcConnectionFactory.java 
244831672 
  
src/test/org/apache/sqoop/db/decorator/TestKerberizedConnectionFactoryDecorator.java
 d3e3fb23e 
  src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java c4caafba5 
  src/test/org/apache/sqoop/hbase/HBaseKerberizedConnectivityTest.java 
3bfb39178 
  src/test/org/apache/sqoop/hbase/HBaseTestCase.java 94b71b61c 
  src/test/org/apache/sqoop/hbase/HBaseUtilTest.java c6a808c33 
  src/test/org/apache/sqoop/hbase/TestHBasePutProcessor.java e78a535f4 
  src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabbb 
  
src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java 
4d2cb2f88 
  src/test/org/apache/sqoop/hive/TestHiveClientFactory.java a3c2dc939 
  src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java 419f888c0 
  src/test/org/apache/sqoop/hive/TestHiveServer2Client.java 02617295e 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 65f079467 
  src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java 410724f37 
  src/test/org/apache/sqoop/hive/TestHiveTypesForAvroTypeMapping.java 276e9eaa4 
  src/test/org/apache/sqoop/hive/TestTableDefWriter.java 626ad22f6 
  src/test/org/apache/sqoop/hive/TestTableDefWriterForExternalTable.java 
f1768ee76 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  src/test/org/apache/sqoop/io/TestCodecMap.java e71921823 
  src/test/org/apache/sqoop/io/TestLobFile.java 2bc95f283 
  src/test/org/apache/sqoop/io/TestNamedFifo.java a93784e08 
  src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java c59aa26ad 
  src/test/org/apache/sqoop/lib/TestBlobRef.java b271d3c7b 
  src/test/org/apache/sqoop/lib/TestBooleanParser.java 914ab37e4 
  src/test/org/apache/sqoop/lib/TestClobRef.java f94d1a8af 
  src/test/org/apache/sqoop/lib/TestFieldFormatter.java 9ac55e703 
  src/test/org/apache/sqoop/lib/TestLargeObjectLoader.java 1e07d7174 
  src/test/org/apache/sqoop/lib/TestRecordParser.java d6844c1cf 
  src/test/org/apache/sqoop/manager/TestDefaultManagerFactory.java

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

2018-11-12 Thread Fero Szabo via Review Board 

---
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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/69060/diff/3-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-12 Thread Fero Szabo via Review Board 


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

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 (?)


- Fero


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


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] [Commented] (SQOOP-3387) Include Column-Remarks

2018-11-12 Thread ASF GitHub Bot (JIRA) 


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

ASF GitHub Bot commented on SQOOP-3387:
---

Github user hatala91 closed the pull request at:

https://github.com/apache/sqoop/pull/49


> Include Column-Remarks
> --
>
> Key: SQOOP-3387
> URL: https://issues.apache.org/jira/browse/SQOOP-3387
> Project: Sqoop
>  Issue Type: Wish
>  Components: connectors, metastore
>Affects Versions: 1.4.7
>Reporter: Tomas Sebastian Hätälä
>Assignee: Tomas Sebastian Hätälä
>Priority: Critical
>  Labels: easy-fix, features, pull-request-available
> Fix For: 1.5.0
>
> Attachments: SQOOP_3387.patch
>
>
> In most RDBMS it is possible to enter comments/ remarks for table and view 
> columns. That way a user can obtain additional information regarding the data 
> and how to use it.
> With the avro file format it would be possible to store this information in 
> the schema file using the "doc"-tag. At the moment this is, however, left 
> blanc.
> Review: https://reviews.apache.org/r/68989/



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

[GitHub] sqoop pull request #49: SQOOP-3387: Add column remarks to avro schema file

2018-11-12 Thread hatala91 
Github user hatala91 closed the pull request at:

https://github.com/apache/sqoop/pull/49


---