You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Anna Szonyi <sz...@cloudera.com> on 2017/07/18 18:20:52 UTC
Re: Review Request 59346: SQOOP-3178 : Incremental Merging for
Parquet File Format
-----------------------------------------------------------
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
>
>
Re: Review Request 59346: SQOOP-3178 : Incremental Merging for
Parquet File Format
Posted by Sandish Kumar HN <sa...@gmail.com>.
> On July 18, 2017, 6:20 p.m., Anna Szonyi wrote:
> > 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
Hi AnnaSzony,
Thanks for the reply and finding issues. I have fixed them this time. Please review it.
- Sandish Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59346/#review180827
-----------------------------------------------------------
On July 18, 2017, 8:53 p.m., Sandish Kumar HN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59346/
> -----------------------------------------------------------
>
> (Updated July 18, 2017, 8:53 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 8b1cba33
> 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 78c7758e
> src/test/com/cloudera/sqoop/TestMerge.java 114e934a
>
>
> Diff: https://reviews.apache.org/r/59346/diff/7/
>
>
> 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
>
>