[HUDI-1969] Support reading logs for MOR Hive rt table

[danny0405]

Tips

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

What is the purpose of the pull request

(For example: This pull request adds quick-start document.)

Brief change log

(for example:)

• Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

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

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end.
• Added HoodieClientWriteTest to verify the change.
• Manually verified the change by running a job locally.

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.

[codecov-commenter]

Codecov Report

Merging #3033 (c80dca3) into master (8c0dbaa) will increase coverage by 0.02%.
The diff coverage is 65.85%.

@@             Coverage Diff              @@
##             master    #3033      +/-   ##
============================================
+ Coverage     47.62%   47.64%   +0.02%     
- Complexity     5502     5505       +3     
============================================
  Files           930      930              
  Lines         41274    41279       +5     
  Branches       4137     4136       -1     
============================================
+ Hits          19657    19668      +11     
+ Misses        19868    19862       -6     
  Partials       1749     1749              

Flag	Coverage Δ
hudicli	39.97% <ø> (ø)
hudiclient	34.58% <ø> (ø)
hudicommon	48.57% <ø> (ø)
hudiflink	59.58% <ø> (ø)
hudihadoopmr	51.76% <65.85%> (+0.47%)	⬆️
hudisparkdatasource	67.32% <ø> (ø)
hudisync	54.48% <ø> (ø)
huditimelineservice	64.07% <ø> (ø)
hudiutilities	58.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...hadoop/realtime/RealtimeCompactedRecordReader.java	76.81% <65.85%> (+11.18%)	⬆️
...apache/hudi/utilities/deltastreamer/DeltaSync.java	71.14% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c0dbaa...c80dca3. Read the comment docs.

[danny0405]

Hi @vinothchandar , can you take a look for this, i found that there was a TODO here, and HUDI has global index now and we can remove the TODO and support reading logs.

[danny0405]

@n3nash Can you take a look, this is important to our flink hive users, thanks in advance.

[hudi-bot]

CI report:

• c80dca3 Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

• @hudi-bot run travis re-run the last Travis build
• @hudi-bot run azure re-run the last Azure build

[vinothchandar]

@danny0405 on it. Going to spend the next week, fully in reviews. So we will move this along

[danny0405]

@danny0405 on it. Going to spend the next week, fully in reviews. So we will move this along

The pure logs file group was never supported before, should we support that in this PR ?

[vinothchandar]

@danny0405 I see less reasons for this outside of HDFS, which can actually take appends. For cloud storage, wrtiing a base file is probably better and now we also have things like clustering which can then control file sizing. So we need not rely only on compaction for e.g

[danny0405]

@danny0405 I see less reasons for this outside of HDFS, which can actually take appends. For cloud storage, wrtiing a base file is probably better and now we also have things like clustering which can then control file sizing. So we need not rely only on compaction for e.g

Thanks @vinothchandar , i know that maybe the cloud storage is in major for North America users, but in China, most of the people still use the HDFS yet, and they still use Hive as the query engine most often, from the perspective of functional integrity, i think we should support it instead of throwing a confusing exception or ignoring the logs silently.

[vinothchandar]

ok makes sense.

[vinothchandar]

Marked this a release blocker, so we can get it into the release.

[vinothchandar]

can you explain why this was needed? This test was already adding inserts into logs? and now with this change we can actually read this.?

[danny0405]

I added 20 more records because the first 100 log records was expected to merge with the initial 100 parquet records, so after merge, we should read out 120 records actually.

[vinothchandar]

Just one comment. The logic seems correct. Can we add tests that explicitly send inserts to logs and see if its all returned correctly

[danny0405]

Just one comment. The logic seems correct. Can we add tests that explicitly send inserts to logs and see if its all returned correctly

Thanks @vinothchandar , before this patch , we assume that each file group has a base parquet fie PLUS logs with only UPDATE records, this is true for BloomFilter index, but with global index like flink, the logs are not only UPDATEs but can also be INSERTs. So the new logic is to read the parquet first then the logs.

The modified test actually tests the case, with first 100 log records to update the old one then another 20 insert records for pure INSERTs.

While this patch does not solve the case to read pure logs file group, because after debugging, i found that this input format found the file groups based on the visible parquet first, the logs are hidden file and be ignored.

I have add one line comment to the test case.

[vinothchandar]

LGTM