----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59346/#review180827 -----------------------------------------------------------
Hi Sandish, Apologies for the delay in the review process, I went through the patch and it is a great contribution and will be very valuable to the users! I have a few review notes on it below, please check them out. Thanks, Anna src/java/org/apache/sqoop/mapreduce/MergeGenericRecordExportMapper.java Lines 50 (patched) <https://reviews.apache.org/r/59346/#comment256130> this seems to be unused in the code. src/java/org/apache/sqoop/mapreduce/MergeGenericRecordExportMapper.java Lines 53 (patched) <https://reviews.apache.org/r/59346/#comment256129> this seems to be an unused variable src/java/org/apache/sqoop/mapreduce/MergeGenericRecordExportMapper.java Lines 58 (patched) <https://reviews.apache.org/r/59346/#comment256131> this seems to be unused. src/java/org/apache/sqoop/mapreduce/MergeJob.java Line 26 (original) <https://reviews.apache.org/r/59346/#comment256111> Thanks for removing these (I assume) unused imports, though it would make sense to remove all unused imports from this class in that case :) src/java/org/apache/sqoop/mapreduce/MergeJob.java Lines 54 (patched) <https://reviews.apache.org/r/59346/#comment256109> this seems to be an unused import. src/java/org/apache/sqoop/mapreduce/MergeJob.java Lines 207-208 (patched) <https://reviews.apache.org/r/59346/#comment256126> public static List<Footer> readFooters(Configuration configuration, Path path) seems to be deprecated, though it doesn't explicitly specify as far as I can tell public static List<Footer> readFooters(Configuration configuration, FileStatus pathStatus, boolean skipRowGroups) this is the replacement method for it, it would make sense to use this instead. src/java/org/apache/sqoop/mapreduce/MergeParquetMapper.java Lines 55 (patched) <https://reviews.apache.org/r/59346/#comment256127> this seems to be an unused variable. src/java/org/apache/sqoop/tool/ImportTool.java Lines 495-499 (patched) <https://reviews.apache.org/r/59346/#comment256106> lastModifiedMergeParquet and lastModifiedMerge have a lot of similarities, it would make sense to refactor them, so we wouldn't have to repeat the code again. src/test/com/cloudera/sqoop/TestMerge.java Lines 57 (patched) <https://reviews.apache.org/r/59346/#comment256110> this seems to be an unused import. - Anna Szonyi On May 31, 2017, 8:25 p.m., Sandish Kumar HN wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59346/ > ----------------------------------------------------------- > > (Updated May 31, 2017, 8:25 p.m.) > > > Review request for Sqoop, Boglarka Egyed, Attila Szabo, and Szabolcs Vasas. > > > Bugs: PARQUET-1010 and SQOOP-3178 > https://issues.apache.org/jira/browse/PARQUET-1010 > https://issues.apache.org/jira/browse/SQOOP-3178 > > > Repository: sqoop-trunk > > > Description > ------- > > New feature for sqoop-1: Sqoop Merge and Incremental Merge for Parquet File > Format > > > Diffs > ----- > > src/java/org/apache/sqoop/mapreduce/MergeGenericRecordExportMapper.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MergeJob.java 8b1cba3 > src/java/org/apache/sqoop/mapreduce/MergeParquetMapper.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/MergeParquetReducer.java PRE-CREATION > src/java/org/apache/sqoop/tool/ImportTool.java 4b1b12d > src/test/com/cloudera/sqoop/TestMerge.java 114e934 > > > Diff: https://reviews.apache.org/r/59346/diff/5/ > > > Testing > ------- > > Hi, > > I have written a Sqoop Merge and Incremental Merge MR for Parquet File > Format and I have tested with million records of data with N number of > iterations. Please review My patch. > > THIS ISSUE HAS DEPENDANCY ON PARQUET BUG. SO I HAVE RESOLVED THE PARQUET > ISSUE AND CREATED A PATCH FOR IT HERE > https://issues.apache.org/jira/browse/PARQUET-1010 > > Please let me know if there are any mistakes in My patch > > > Thanks, > > Sandish Kumar HN > >
