[HUDI-3206] Unify Hive's MOR implementations to avoid duplication #4559

alexeykudinkin · 2022-01-11T02:23:27Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contribute/how-to-contribute before opening a pull request.

What is the purpose of the pull request

Unify Hive's MOR implementations to avoid duplication to avoid duplication across implementations for different file-formats (Parquet, HFile, etc)

Brief change log

Extracted HoodieRealtimeFileInputFormatBase (extending COW HoodieFileInputFormatBase base)
Rebased Parquet, HFile implementations onto HoodieRealtimeFileInputFormatBase
Tidying up

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

garyli1019 · 2022-01-27T02:33:51Z

hi @alexeykudinkin , a high level question. Is this approach using the hudi metadata table to fetch latest file? That would be great if we can get rid of file listing while unifying the code path for reader.

alexeykudinkin · 2022-01-27T03:23:59Z

@garyli1019 Hive is using MT as of #4531

…e` (those are not file-format specific)

… MOR functionality

alexeykudinkin · 2022-02-04T00:42:53Z

@hudi-bot run azure

alexeykudinkin · 2022-02-04T17:24:48Z

...k-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestColumnStatsIndex.scala

-      val (uuid, idx) = uuids.zipWithIndex.find { case (uuid, _) => fileName.contains(uuid) }.get
-      fileName.replace(uuid, idx.toString)
+      val uuid = uuids.find(uuid => fileName.contains(uuid)).get
+      fileName.replace(uuid, "xxx")


This is to fix test flakiness

hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieFileInputFormatBase.java

nsivabalan · 2022-02-04T22:04:25Z

hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieFileInputFormatBase.java

+  protected FileSplit makeSplit(Path file, long start, long length,
+                                String[] hosts, String[] inMemoryHosts) {
+    FileSplit split = new FileSplit(file, start, length, hosts, inMemoryHosts);
+    if (file instanceof PathWithBootstrapFileStatus) {


can you help me understand why don't I see diff FileStatuses here like RealtimeFileStatus for eg, but just PathWithBootstrapFileStatus?

My understanding is that these methods are moved from HoodieParquetInputFormat. @alexeykudinkin is the logic parquet agnostic and going to be reused by other file formats like HFile? I think if that's not the case, better to keep them in HoodieParquetInputFormat, since this base class is going to be general for different formats.

This is used only for COW

@alexeykudinkin what about my above question regarding whether the logic should reside in HoodieParquetInputFormat or this class?

@yihua GH comments are weird, when i was responding to @nsivabalan there were no comments of yours, and then it sandwiched your comment in b/w of those.

My understanding is that these methods are moved from HoodieParquetInputFormat. @alexeykudinkin is the logic parquet agnostic and going to be reused by other file formats like HFile? I think if that's not the case, better to keep them in HoodieParquetInputFormat, since this base class is going to be general for different formats.

Your understanding is correct, all of this logic is file-format agnostic.

sounds good!

@alexeykudinkin no worries. That sg.

nsivabalan · 2022-02-04T22:16:15Z

...-hadoop-mr/src/main/java/org/apache/hudi/hadoop/realtime/HoodieHFileRealtimeInputFormat.java

-    return timeline;
-  }
+  // NOTE: We're only using {@code HoodieHFileInputFormat} to compose {@code RecordReader}
+  private final HoodieHFileInputFormat hFileInputFormat = new HoodieHFileInputFormat();


There are quite a few things different in HFileRealtimeInputFormat compared to ParquetRealtimeInputFormat. for eg, getSplits just does HoodieRealtimeInputFormatUtils.getRealtimeSplits(job, fileSplits) in case of HFile, where as in case of Parquet, we do both realtimeSplits and incrementalSplits.
and then

@Override protected boolean includeLogFilesForSnapshotView() { return false; }

@Override protected boolean isSplitable(FileSystem fs, Path filename) { // This file isn't splittable. return false; }

these are diff in parquet real time IF.

filterInstantsTimeline(HoodieDefaultTimeline timeline) is empty in case of HFileRTIF. after unification, I don't see these are overridden here in HoodieHFileRealtimeInputFormat.

can you help me understand please.

In principle, COW/MOR handling should be invariant of file-format used underneath.

Good catch w/ isSplitable

I get it. but in master, we disabled log files for HFile realtime IF and now we are enabling is it?
excerpt from HoodieHFileInputFormat in master.

@Override protected boolean includeLogFilesForSnapshotView() { return true; }

hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieFileInputFormatBase.java

yihua · 2022-02-04T23:24:19Z

hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieFileInputFormatBase.java


-  protected abstract boolean includeLogFilesForSnapshotView();
+  @Override
+  protected FileSplit makeSplit(Path file, long start, long length,


nit: put two makeSplit() methods together?

Will clean this up in the final PR #4743

yihua · 2022-02-04T23:39:59Z

hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieFileInputFormatBase.java

+  protected FileSplit makeSplit(Path file, long start, long length,
+                                String[] hosts, String[] inMemoryHosts) {
+    FileSplit split = new FileSplit(file, start, length, hosts, inMemoryHosts);
+    if (file instanceof PathWithBootstrapFileStatus) {


My understanding is that these methods are moved from HoodieParquetInputFormat. @alexeykudinkin is the logic parquet agnostic and going to be reused by other file formats like HFile? I think if that's not the case, better to keep them in HoodieParquetInputFormat, since this base class is going to be general for different formats.

yihua · 2022-02-05T00:00:06Z

hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieFileInputFormatBase.java

    return targetFiles;
  }
+
+  private void validate(List<FileStatus> targetFiles, List<FileStatus> legacyFileStatuses) {


Are the methods below merely moved? Looks like the order changes.

Yeah, GJF rule is applied

(non-static methods)
public
protected
private

(static methods)
public
protected
private

yihua · 2022-02-05T00:24:50Z

hudi-spark-datasource/hudi-spark/src/test/resources/index/zorder/z-index-table-merged.json

-{"c1_maxValue":486,"c1_minValue":59,"c1_num_nulls":0,"c2_maxValue":" 79sdc","c2_minValue":" 111sdc","c2_num_nulls":0,"c3_maxValue":771.590,"c3_minValue":82.111,"c3_num_nulls":0,"c5_maxValue":50,"c5_minValue":7,"c5_num_nulls":0,"c6_maxValue":"2020-11-21","c6_minValue":"2020-01-22","c6_num_nulls":0,"c7_maxValue":"5g==","c7_minValue":"Ow==","c7_num_nulls":0,"c8_maxValue":9,"c8_minValue":9,"c8_num_nulls":0,"file":"part-00002-1-c000.snappy.parquet"}
-{"c1_maxValue":959,"c1_minValue":0,"c1_num_nulls":0,"c2_maxValue":" 959sdc","c2_minValue":" 0sdc","c2_num_nulls":0,"c3_maxValue":916.697,"c3_minValue":19.000,"c3_num_nulls":0,"c5_maxValue":97,"c5_minValue":1,"c5_num_nulls":0,"c6_maxValue":"2020-11-22","c6_minValue":"2020-01-01","c6_num_nulls":0,"c7_maxValue":"yA==","c7_minValue":"AA==","c7_num_nulls":0,"c8_maxValue":9,"c8_minValue":9,"c8_num_nulls":0,"file":"part-00003-0-c000.snappy.parquet"}
-{"c1_maxValue":272,"c1_minValue":8,"c1_num_nulls":0,"c2_maxValue":" 8sdc","c2_minValue":" 129sdc","c2_num_nulls":0,"c3_maxValue":979.272,"c3_minValue":430.129,"c3_num_nulls":0,"c5_maxValue":28,"c5_minValue":2,"c5_num_nulls":0,"c6_maxValue":"2020-11-20","c6_minValue":"2020-03-23","c6_num_nulls":0,"c7_maxValue":"8A==","c7_minValue":"Ag==","c7_num_nulls":0,"c8_maxValue":9,"c8_minValue":9,"c8_num_nulls":0,"file":"part-00003-1-c000.snappy.parquet"}
+{"c1_maxValue":272,"c1_minValue":8,"c1_num_nulls":0,"c2_maxValue":" 8sdc","c2_minValue":" 129sdc","c2_num_nulls":0,"c3_maxValue":979.272,"c3_minValue":430.129,"c3_num_nulls":0,"c5_maxValue":28,"c5_minValue":2,"c5_num_nulls":0,"c6_maxValue":"2020-11-20","c6_minValue":"2020-03-23","c6_num_nulls":0,"c7_maxValue":"8A==","c7_minValue":"Ag==","c7_num_nulls":0,"c8_maxValue":9,"c8_minValue":9,"c8_num_nulls":0,"file":"part-00003-xxx-c000.snappy.parquet"}


Why are these two files changed? To fix test flakyness?

alexeykudinkin · 2022-02-05T02:28:06Z

@hudi-bot run azure

hudi-bot · 2022-02-05T03:38:10Z

CI report:

47970bd UNKNOWN
8849116 UNKNOWN
3a9315b UNKNOWN
8b7dc5f Azure: FAILURE Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

yihua

LGTM. Are there already unit/functional tests around HFile input format?

nsivabalan

LGTM. check if we have tests for HFileInputFormat (regular and realtime). If not, can you file a tracking jira and take it up later. lets proceed w/ landing all stacked PRs.

alexeykudinkin · 2022-02-07T20:34:08Z

@nsivabalan we do have tests for all input formats (Parquet, HFile, Orc)

…ache#4559) Unify Hive's MOR implementations to avoid duplication to avoid duplication across implementations for different file-formats (Parquet, HFile, etc) - Extracted HoodieRealtimeFileInputFormatBase (extending COW HoodieFileInputFormatBase base) - Rebased Parquet, HFile implementations onto HoodieRealtimeFileInputFormatBase - Tidying up

alexeykudinkin force-pushed the ak/rpath-ref-5 branch 7 times, most recently from a70ea22 to 7798caf Compare January 14, 2022 23:16

alexeykudinkin force-pushed the ak/rpath-ref-5 branch 2 times, most recently from 060a816 to b557e6b Compare January 19, 2022 21:35

alexeykudinkin mentioned this pull request Jan 19, 2022

[HUDI-3191] Rebasing Hive's FileInputFormat onto AbstractHoodieTableFileIndex #4531

Merged

5 tasks

alexeykudinkin force-pushed the ak/rpath-ref-5 branch 2 times, most recently from 0aa3cea to e68070c Compare January 21, 2022 20:22

yihua self-assigned this Jan 21, 2022

alexeykudinkin force-pushed the ak/rpath-ref-5 branch from e68070c to 8a5f12f Compare January 25, 2022 00:04

alexeykudinkin force-pushed the ak/rpath-ref-5 branch from c18bd33 to 6d402f1 Compare February 2, 2022 21:15

Alexey Kudinkin added 11 commits February 3, 2022 14:31

Leverage appropriate RealtimeFileStatus objects for MOR tables

329264c

Bubbled up makeSplit, isSplittable into `HoodieFileInputFormatBas…

9562f90

…e` (those are not file-format specific)

Extracted HoodieRealtimeFileInputFormatBase w/ file-format-agnostic…

b4b95e5

… MOR functionality

Rebased HFile, Parquet impls onto HoodieRealtimeFileInputFormatBase

c7b2c99

Tidying up

6c01a5e

Fixing compilation

9fdb92e

Unused imports

9bf09e0

Missing license

5a3c7b2

After rebase fixes

16d9fc2

Tidying up

761fa9c

Fixed compilation

b2598cb

alexeykudinkin force-pushed the ak/rpath-ref-5 branch from 6d402f1 to b2598cb Compare February 3, 2022 22:31

alexeykudinkin changed the title ~~[HUDI-3206][Stacked on 4556] Unify Hive's MOR implementations to avoid duplication~~ [HUDI-3206] Unify Hive's MOR implementations to avoid duplication Feb 3, 2022

alexeykudinkin force-pushed the ak/rpath-ref-5 branch from 8849116 to 3a9315b Compare February 4, 2022 05:51

Fixed flaky test

0a7342e

alexeykudinkin force-pushed the ak/rpath-ref-5 branch from 3a9315b to 0a7342e Compare February 4, 2022 06:09

alexeykudinkin commented Feb 4, 2022

View reviewed changes

nsivabalan requested changes Feb 4, 2022

View reviewed changes

Missing isSplitable override for HFileRealtimeInputFormat

8b7dc5f

yihua reviewed Feb 5, 2022

View reviewed changes

yihua approved these changes Feb 7, 2022

View reviewed changes

nsivabalan approved these changes Feb 7, 2022

View reviewed changes

nsivabalan merged commit 3f263b8 into apache:master Feb 7, 2022

[HUDI-3206] Unify Hive's MOR implementations to avoid duplication #4559

[HUDI-3206] Unify Hive's MOR implementations to avoid duplication #4559

Uh oh!

Conversation

alexeykudinkin commented Jan 11, 2022

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

garyli1019 commented Jan 27, 2022

Uh oh!

alexeykudinkin commented Jan 27, 2022

Uh oh!

alexeykudinkin commented Feb 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihua Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeykudinkin commented Feb 5, 2022

Uh oh!

hudi-bot commented Feb 5, 2022

CI report:

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

alexeykudinkin commented Feb 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yihua Feb 5, 2022 •

edited

Loading