[HUDI-728]: Implement custom key generator #1433

pratyakshsharma · 2020-03-21T09:20:33Z

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

We have TimestampBasedKeyGenerator for defining custom partition paths and we have ComplexKeyGenerator for supporting having combination of fields as record key or partition key.

However we do not have support for the case where one wants to have combination of fields as record key along with being able to define custom partition paths.
This PR aims to give a generic implementation where we can define key generator for every field in partition path.

Brief change log

Introduced PartitionKeyType in KeyGenerator, also added 2 new functions
String getPartitionPath(GenericRecord record, String partitionPathField)
String getRecordKey(GenericRecord record)
Introduced a new class CustomKeyGenerator which accepts input for partition path field in form -> field1:PartitionKeyType1,field2:PartitionKeyType2
All the corner cases have been handled. Added a test class TestCustomKeyGenerator with only one test case for now. Will be adding more.

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.

pratyakshsharma · 2020-03-21T09:31:39Z

This PR is a currently work in progress. Will be adding more test cases to ensure everything works fine.

pratyakshsharma · 2020-03-21T09:37:22Z

@bvaradar @vinothchandar please have a look and let me know if this looks good.

vinothchandar · 2020-03-22T22:07:11Z

@nsivabalan could you review this please?

vinothchandar · 2020-03-30T20:25:58Z

@pratyakshsharma thanks for your patience.. I will review this myself and get back in a day.

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/KeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

nsivabalan · 2020-03-30T22:05:26Z

@vinothchandar : my bad. I have taken a look. You can review once my feedback is addressed. Will keep you posted.

pratyakshsharma · 2020-04-14T20:09:10Z

@vinothchandar @nsivabalan please take pass.

nsivabalan · 2020-04-19T16:37:35Z

LGTM. @vinothchandar : do you want to review or can we go ahead and merge it.

pratyakshsharma · 2020-04-22T12:05:21Z

@vinothchandar Let us close this? :)

vinothchandar · 2020-04-22T20:47:04Z

@pratyakshsharma sorry .. fell off my radar since I was not an assignee.. I do have some concerns.. Review coming by your day time :)

vinothchandar

I have some concerns on the implementation.. Also I think you may have checked in a few parquet files from testing?

hudi-spark/src/main/java/org/apache/hudi/keygen/KeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/ComplexKeyGenerator.java

vinothchandar · 2020-04-24T05:31:58Z

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

+ * The complete partition path is created as <value for field1 basis PartitionKeyType1>/<value for field2 basis PartitionKeyType2> and so on.
+ *
+ * Few points to consider:
+ * 1. If you want to customise some partition path field on a timestamp basis, you can use field1:timestampBased


all this needs to be documented for the user?

in our documentation you mean to say? Happy to do that @vinothchandar
Will raise a follow up PR once this gets merged.

yes..

is the CustomKeyGenerator compatible with the SimpleKeyGenerator configs? I am wondering if we can replace the default with this, without forcing user to do any additional work.. I think this is worth pursuing.. (We can then rename this DefaultKeyGenerator

No the configs are not compatible, since CustomKeyGenerator expects partitionPathFields to be provided in a particular format. :(
But since we are going with a major release next, I guess we can make this as the default? (My thinking here is users can expect a bit of breaking changes in major releases, anyways we will mention all the changes in the release notes). WDYT?

Also because this key generator pretty much covers all the possible cases for key generation.

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

pratyakshsharma · 2020-04-24T09:39:08Z

I have some concerns on the implementation.. Also I think you may have checked in a few parquet files from testing?

My bad. Will remove them.

codecov-io · 2020-04-24T10:48:22Z

Codecov Report

Merging #1433 into master will decrease coverage by 0.29%.
The diff coverage is 35.80%.

@@             Coverage Diff              @@
##             master    #1433      +/-   ##
============================================
- Coverage     71.66%   71.37%   -0.30%     
+ Complexity      294      289       -5     
============================================
  Files           378      379       +1     
  Lines         16551    16603      +52     
  Branches       1670     1674       +4     
============================================
- Hits          11861    11850      -11     
- Misses         3959     4029      +70     
+ Partials        731      724       -7

Impacted Files	Coverage Δ	Complexity Δ
...e/hudi/exception/HoodieDeltaStreamerException.java	`0.00% <ø> (ø)`	`0.00 <0.00> (?)`
...ava/org/apache/hudi/keygen/CustomKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...main/java/org/apache/hudi/keygen/KeyGenerator.java	`60.00% <0.00%> (-40.00%)`	`0.00 <0.00> (ø)`
...apache/hudi/keygen/TimestampBasedKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...apache/hudi/utilities/deltastreamer/DeltaSync.java	`72.44% <ø> (ø)`	`37.00 <0.00> (ø)`
...va/org/apache/hudi/keygen/ComplexKeyGenerator.java	`92.30% <94.44%> (+5.46%)`	`0.00 <0.00> (ø)`
...g/apache/hudi/keygen/GlobalDeleteKeyGenerator.java	`91.30% <100.00%> (+0.82%)`	`0.00 <0.00> (ø)`
...ava/org/apache/hudi/keygen/SimpleKeyGenerator.java	`90.47% <100.00%> (+2.24%)`	`0.00 <0.00> (ø)`
... and 1 more

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 19cc15c...f5b7e4b. Read the comment docs.

pratyakshsharma · 2020-04-24T12:27:48Z

@vinothchandar Have tried to address the comments, please have a look again. Also looking at the Codecov report, I feel there is a need to add test cases for all key generators for which we do not have right now like SimpleKeyGenerator, ComplexKeyGenerator etc. Should I include that in this PR or should I go for a separate PR. Please suggest.

vinothchandar · 2020-04-27T20:05:20Z

Should I include that in this PR or should I go for a separate PR. Please suggest.

Your call.. Doing it here is fine by me as well, since you are touching all those files anyway..

vinothchandar

Few comments around usability..

vinothchandar · 2020-04-27T20:25:14Z

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

+ * The complete partition path is created as <value for field1 basis PartitionKeyType1>/<value for field2 basis PartitionKeyType2> and so on.
+ *
+ * Few points to consider:
+ * 1. If you want to customise some partition path field on a timestamp basis, you can use field1:timestampBased


yes..

is the CustomKeyGenerator compatible with the SimpleKeyGenerator configs? I am wondering if we can replace the default with this, without forcing user to do any additional work.. I think this is worth pursuing.. (We can then rename this DefaultKeyGenerator

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/GlobalDeleteKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java

pratyakshsharma · 2020-05-01T16:19:49Z

@vinothchandar Please take a pass.

vinothchandar · 2020-05-05T15:16:59Z

@pratyakshsharma rebase again? I can take a final pass

vinothchandar · 2020-05-14T20:35:53Z

@pratyakshsharma Rebased and removed the parquet files etc..

vinothchandar · 2020-05-14T20:37:19Z

@nsivabalan can you shepherd this one home from here>

nsivabalan

@pratyakshsharma : have left few comments. I see that this has been going on for quite some time, partly because of delay in reviews. I will ensure I keep a tab at it. ping me once you have addressed the comments.

hudi-spark/src/main/java/org/apache/hudi/keygen/ComplexKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/SimpleKeyGenerator.java

nsivabalan · 2020-05-15T03:35:15Z

hudi-spark/src/main/java/org/apache/hudi/keygen/SimpleKeyGenerator.java

+    return partitionPath;
+  }
+
+  public String getRecordKey(GenericRecord record) {


did you think if we need to make this an abstract method in KeyGenerator?

Yeah this has been discussed already. Please refer to this - #1433 (comment)

hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

pratyakshsharma · 2020-05-17T12:14:27Z

@nsivabalan I have tried to include the changes from #1597 as well in this. Please take a pass.

nsivabalan

would have been easier to have it in two different diffs. I need to review that patch now. It is fine. But can you help me with something. Can you point me to exact commits where you addressed my last set of comments and commits where you pulled in the other PR? When I looked at the last 3 commits, it was bit confusing to me.

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java

hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java

pratyakshsharma · 2020-05-18T15:59:18Z

Can you point me to exact commits where you addressed my last set of comments and commits where you pulled in the other PR?

Sure. I addressed your last set of comments in - 4ebd505

I pulled the other PR in - cfe94e4.

Hope that helps.

codecov-commenter · 2020-05-20T15:07:59Z

Codecov Report

Merging #1433 into master will increase coverage by 1.52%.
The diff coverage is 8.45%.

@@             Coverage Diff              @@
##             master    #1433      +/-   ##
============================================
+ Coverage     16.71%   18.24%   +1.52%     
- Complexity      795      854      +59     
============================================
  Files           340      347       +7     
  Lines         15030    15257     +227     
  Branches       1499     1525      +26     
============================================
+ Hits           2512     2783     +271     
+ Misses        12188    12122      -66     
- Partials        330      352      +22

Impacted Files	Coverage Δ	Complexity Δ
...e/hudi/exception/HoodieDeltaStreamerException.java	`0.00% <ø> (ø)`	`0.00 <0.00> (?)`
...va/org/apache/hudi/keygen/ComplexKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...ava/org/apache/hudi/keygen/CustomKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...g/apache/hudi/keygen/GlobalDeleteKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...apache/hudi/keygen/NonpartitionedKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...apache/hudi/keygen/TimestampBasedKeyGenerator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ava/org/apache/hudi/keygen/SimpleKeyGenerator.java	`73.68% <75.00%> (+14.86%)`	`0.00 <0.00> (ø)`
...ain/java/org/apache/hudi/avro/HoodieAvroUtils.java	`48.09% <0.00%> (-5.88%)`	`22.00% <0.00%> (ø%)`
...in/scala/org/apache/hudi/AvroConversionUtils.scala	`45.45% <0.00%> (-4.55%)`	`0.00% <0.00%> (ø%)`
...c/main/java/org/apache/hudi/index/HoodieIndex.java	`36.84% <0.00%> (-4.34%)`	`3.00% <0.00%> (ø%)`
... and 36 more

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 25a0080...5f0ad60. Read the comment docs.

nsivabalan

Guess you have added some parquet files by mistake. Rest looks good except for one minor comment. Once done, do squash all your commits to one. We can merge it.

hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java

hudi-spark/src/test/java/org/apache/hudi/keygen/TestComplexKeyGenerator.java

hudi-spark/src/test/java/org/apache/hudi/keygen/TestCustomKeyGenerator.java

hudi-spark/src/test/java/org/apache/hudi/keygen/TestSimpleKeyGenerator.java

hudi-spark/src/test/java/org/apache/hudi/keygen/TestTimestampBasedKeyGenerator.java

nsivabalan · 2020-05-22T12:52:38Z

@pratyakshsharma : can you fix the build issue.

pratyakshsharma · 2020-05-22T16:19:41Z

@nsivabalan I squashed the commits and force pushed after unstaging the 2 parquet files, but they are still showing.

vinothchandar · 2020-06-22T22:21:52Z

cc @wangxianghu ..

@pratyakshsharma confirmed, he will resume this wiork and take it across finish line��

pratyakshsharma · 2020-06-27T21:23:47Z

@nsivabalan can we merge this now?

nsivabalan

LGTM. can you squash all commits and let me know.

pratyakshsharma · 2020-07-01T17:43:11Z

LGTM. can you squash all commits and let me know

I have already squashed @nsivabalan :)

pratyakshsharma · 2020-07-09T06:33:47Z

Pinging to see if you have any more concerns here or we can merge this @nsivabalan ? :)

vinothchandar assigned bvaradar and nsivabalan Mar 22, 2020

nsivabalan requested changes Mar 30, 2020

View reviewed changes

nsivabalan approved these changes Apr 19, 2020

View reviewed changes

vinothchandar assigned vinothchandar and unassigned bvaradar Apr 22, 2020

vinothchandar reviewed Apr 24, 2020

View reviewed changes

pratyakshsharma force-pushed the hudi-728 branch from b6d4314 to 454116a Compare April 24, 2020 10:05

vinothchandar reviewed Apr 27, 2020

View reviewed changes

vinothchandar mentioned this pull request May 7, 2020

[WIP] Added a MultiFormatTimestampBasedKeyGenerator that allows for multipl… #1597

Closed

vinothchandar self-requested a review May 14, 2020 19:33

vinothchandar force-pushed the hudi-728 branch from 0b1dd6c to bf90715 Compare May 14, 2020 20:35

vinothchandar removed their assignment May 14, 2020

nsivabalan requested changes May 15, 2020

View reviewed changes

nsivabalan reviewed May 17, 2020

View reviewed changes

hudi-spark/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java Outdated Show resolved Hide resolved

hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java Show resolved Hide resolved

nsivabalan reviewed May 21, 2020

View reviewed changes

pratyakshsharma force-pushed the hudi-728 branch from 34e9925 to 5f6afd5 Compare May 22, 2020 10:17

wangxianghu mentioned this pull request Jun 22, 2020

[HUDI-1027] Introduce TimestampBasedComplexKeyGenerator to support ti… #1744

Closed

5 tasks

pratyakshsharma force-pushed the hudi-728 branch from 9cab65f to 2810271 Compare June 27, 2020 21:22

nsivabalan approved these changes Jul 1, 2020

View reviewed changes

[HUDI-728]: implemented custom key generator

f3094f6

pratyakshsharma force-pushed the hudi-728 branch from 2810271 to f3094f6 Compare July 1, 2020 11:36

nsivabalan merged commit c7f1a78 into apache:master Jul 9, 2020

[HUDI-728]: Implement custom key generator #1433

[HUDI-728]: Implement custom key generator #1433

Conversation

pratyakshsharma commented Mar 21, 2020 • edited

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

pratyakshsharma commented Mar 21, 2020

pratyakshsharma commented Mar 21, 2020

vinothchandar commented Mar 22, 2020

vinothchandar commented Mar 30, 2020

nsivabalan commented Mar 30, 2020

pratyakshsharma commented Apr 14, 2020

nsivabalan commented Apr 19, 2020

pratyakshsharma commented Apr 22, 2020

vinothchandar commented Apr 22, 2020

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented Apr 24, 2020

codecov-io commented Apr 24, 2020

Codecov Report

pratyakshsharma commented Apr 24, 2020

vinothchandar commented Apr 27, 2020

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented May 1, 2020

vinothchandar commented May 5, 2020

vinothchandar commented May 14, 2020

vinothchandar commented May 14, 2020

nsivabalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented May 17, 2020

nsivabalan left a comment

Choose a reason for hiding this comment

pratyakshsharma commented May 18, 2020

codecov-commenter commented May 20, 2020 • edited

Codecov Report

nsivabalan left a comment

Choose a reason for hiding this comment

nsivabalan commented May 22, 2020

pratyakshsharma commented May 22, 2020

vinothchandar commented Jun 22, 2020

pratyakshsharma commented Jun 27, 2020

nsivabalan left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Jul 1, 2020

pratyakshsharma commented Jul 9, 2020

pratyakshsharma commented Mar 21, 2020 •

edited

codecov-commenter commented May 20, 2020 •

edited