> On Nov. 9, 2018, 2:26 p.m., Boglarka Egyed wrote: > > src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java > > Lines 220 (patched) > > <https://reviews.apache.org/r/69060/diff/3/?file=2106486#file2106486line224> > > > > 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 > >