[HUDI-2656] Generalize HoodieIndex for flexible record data type #3893

yihua · 2021-10-29T15:47:09Z

Make HoodieRecord abstract and use HoodieAvroRecord for the implementation for extensibility
Make HoodieIndex independent of HoodieRecordPayload

alexeykudinkin · 2021-12-17T16:43:08Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieAvroRecord.java

+
+package org.apache.hudi.common.model;
+
+public class HoodieAvroRecord<T extends HoodieRecordPayload> extends HoodieRecord<T> {


Should be HoodieRecordPayload<T>

Right. This is more like a retrofit for Java 7 style and users extending old HoodieRecord. This should be fixed in one shot with new APIs.

@yihua sorry, not sure i understood your point. Can you elaborate?

Why do we want to extend HoodieAvroRecord with format-specific impl?

Discussed offline: this will be an intermediate state until RFC-46 is fully implemented

As discussed, this is more of an intermediate solution for row writer, before RFC-46 revamps it completely.

alexeykudinkin · 2021-12-17T16:43:45Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecordPayloadDelegate.java

+import java.io.Serializable;
+import java.util.Properties;
+
+public interface HoodieRecordPayloadDelegate<T> extends Serializable {


Can you please add a java-doc to explain purpose of this class?

The idea of adding HoodieRecordPayloadDelegate is the same as the recording combining/merging API, i.e., decoupling the merging logic from record payload and adding an independent interface for merging, taking two type-generic instances and returning the result instance.

since this class is not used in this PR and it'll probably be replaced with new API like Mergeable, I'll remove it from this patch.

xushiyan · 2022-01-22T19:09:18Z

...lient/hudi-spark-client/src/main/java/org/apache/hudi/index/hbase/SparkHoodieHBaseIndex.java

-  public Map<String, Integer> mapFileWithInsertsToUniquePartition(JavaRDD<WriteStatus> writeStatusRDD) {
+  Map<String, Integer> mapFileWithInsertsToUniquePartition(JavaRDD<WriteStatus> writeStatusRDD) {


this method only used in test; changing to package access due to using RDD in the signature

Since we don't want to depend on Guava we can add our own annotation with the same purpose as @VisibleForTesting

xushiyan · 2022-01-22T19:09:25Z

...lient/hudi-spark-client/src/main/java/org/apache/hudi/index/hbase/SparkHoodieHBaseIndex.java

-  public Tuple2<Long, Integer> getHBasePutAccessParallelism(final JavaRDD<WriteStatus> writeStatusRDD) {
+  Tuple2<Long, Integer> getHBasePutAccessParallelism(final JavaRDD<WriteStatus> writeStatusRDD) {


xushiyan · 2022-01-22T19:16:01Z

...t/hudi-spark-client/src/test/java/org/apache/hudi/index/hbase/TestSparkHoodieHBaseIndex.java

 */

-package org.apache.hudi.client.functional;
+package org.apache.hudi.index.hbase;


moved to the same package as SparkHoodieHBaseIndex so that some methods with RDD in the signature can be changed to package access

xushiyan · 2022-01-22T22:31:56Z

...hudi-client-common/src/main/java/org/apache/hudi/index/bloom/BaseHoodieBloomIndexHelper.java

      HoodieWriteConfig config, HoodieEngineContext context, HoodieTable hoodieTable,
      HoodiePairData<String, String> partitionRecordKeyPairs,
-      HoodieData<ImmutablePair<String, HoodieKey>> fileComparisonPairs,
+      HoodieData<Pair<String, HoodieKey>> fileComparisonPairs,


hide implementation from public API

xushiyan · 2022-01-22T22:33:50Z

hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/index/FlinkHoodieIndex.java

  @Override
  @PublicAPIMethod(maturity = ApiMaturityLevel.EVOLVING)
  public <R> HoodieData<HoodieRecord<R>> tagLocation(
      HoodieData<HoodieRecord<R>> records, HoodieEngineContext context,
      HoodieTable hoodieTable) throws HoodieIndexException {
-    return HoodieList.of(tagLocation(
-        HoodieList.getList(records.map(record -> (HoodieRecord<T>) record)), context, hoodieTable));
+    List<HoodieRecord<T>> hoodieRecords = tagLocation(HoodieList.getList(records.map(record -> (HoodieRecord<T>) record)), context, hoodieTable);
+    return HoodieList.of(hoodieRecords.stream().map(r -> (HoodieRecord<R>) r).collect(Collectors.toList()));
  }


@yihua don't seem to find a better way than this double casting.

I think this is fine since this is more like an intermediate fix. Once RFC-46 is fully done, the logic here should be much cleaner without generics.

alexeykudinkin · 2022-01-25T00:26:26Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndexUtils.java

   */
  public static HoodieRecord getTaggedRecord(HoodieRecord inputRecord, Option<HoodieRecordLocation> location) {
-    HoodieRecord record = inputRecord;
+    HoodieRecord<?> record = inputRecord;


Thanks for fixing this!

alexeykudinkin · 2022-01-25T00:30:24Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieAvroRecord.java

+
+package org.apache.hudi.common.model;
+
+public class HoodieAvroRecord<T extends HoodieRecordPayload> extends HoodieRecord<T> {


@yihua sorry, not sure i understood your point. Can you elaborate?

Why do we want to extend HoodieAvroRecord with format-specific impl?

alexeykudinkin · 2022-01-25T00:48:24Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecord.java

 * A Single Record managed by Hoodie.
 */
-public class HoodieRecord<T extends HoodieRecordPayload> implements Serializable {
+public abstract class HoodieRecord<T> implements Serializable {


@yihua @xushiyan let's chat more on this to make sure we're aligned on the approach going f/w:

I was thinking of keeping this component file-format agnostic and instead make it engine-specific, while refactoring MOR table read-path for efficient querying.

Can you elaborate what's the goal you're striving for w/ HoodieAvroRecord?

P.S. Putting this context in here for somebody who might not be aware of previous conversations

@alexeykudinkin so the main goal here is to make HoodieRecord independent of HoodieRecordPayload, this will allow HoodieRowRecord to be used by row-writer code path. HoodieAvroRecord is also meant for compatibility with existing codebase and ease the decoupling. Yea we should discuss more about HoodieAvroRecord vs SparkHoodieRecord.

Discussed offline: this will be an intermediate state until RFC-46 is fully implemented

alexeykudinkin · 2022-01-31T17:47:20Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndex.java

  @PublicAPIMethod(maturity = ApiMaturityLevel.DEPRECATED)
  public I tagLocation(I records, HoodieEngineContext context,
-                       HoodieTable<T, I, K, O> hoodieTable) throws HoodieIndexException {
+                       HoodieTable hoodieTable) throws HoodieIndexException {


As general rule of thumb, i think we should aim to fix all raw-types occurrences in the code-base.

Can you please fix new instances so that we at least don't add to our debt?

@alexeykudinkin Yes, i plan to make a follow up PR to remove T from HoodieTable, then i can make the interface taking HoodieTable<I, K, O>.

@xushiyan @alexeykudinkin Since this is anyway backward incompatible, should we just remove the deprecated public API methods and get rid of I and O as well? The reason to keep these methods and the generics is to adapt for users extending these APIs. If you want to change the generics, I'd prefer that all such generics changes in relation to all public APIs should get in 0.11.0 release in one shot.

The deprecated APIs are going to be removed in a separate PR to keep the scope of this PR limited.

alexeykudinkin · 2022-01-31T17:47:42Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndex.java

  @PublicAPIMethod(maturity = ApiMaturityLevel.DEPRECATED)
  public O updateLocation(O writeStatuses, HoodieEngineContext context,
-                          HoodieTable<T, I, K, O> hoodieTable) throws HoodieIndexException {
+                          HoodieTable hoodieTable) throws HoodieIndexException {


Here as well

alexeykudinkin · 2022-01-31T17:50:10Z

...lient/hudi-spark-client/src/main/java/org/apache/hudi/index/hbase/SparkHoodieHBaseIndex.java

-  public Map<String, Integer> mapFileWithInsertsToUniquePartition(JavaRDD<WriteStatus> writeStatusRDD) {
+  Map<String, Integer> mapFileWithInsertsToUniquePartition(JavaRDD<WriteStatus> writeStatusRDD) {


Since we don't want to depend on Guava we can add our own annotation with the same purpose as @VisibleForTesting

alexeykudinkin · 2022-01-31T17:51:35Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieAvroRecord.java

+
+package org.apache.hudi.common.model;
+
+public class HoodieAvroRecord<T extends HoodieRecordPayload> extends HoodieRecord<T> {


Discussed offline: this will be an intermediate state until RFC-46 is fully implemented

alexeykudinkin · 2022-01-31T17:52:01Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieRecord.java

 * A Single Record managed by Hoodie.
 */
-public class HoodieRecord<T extends HoodieRecordPayload> implements Serializable {
+public abstract class HoodieRecord<T> implements Serializable {


Discussed offline: this will be an intermediate state until RFC-46 is fully implemented

yihua

Overall LGTM. Good job on revamping this! I put up one major call we need to make and a few nits.

yihua · 2022-01-31T18:36:02Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndex.java

  @PublicAPIMethod(maturity = ApiMaturityLevel.DEPRECATED)
  public I tagLocation(I records, HoodieEngineContext context,
-                       HoodieTable<T, I, K, O> hoodieTable) throws HoodieIndexException {
+                       HoodieTable hoodieTable) throws HoodieIndexException {


@xushiyan @alexeykudinkin Since this is anyway backward incompatible, should we just remove the deprecated public API methods and get rid of I and O as well? The reason to keep these methods and the generics is to adapt for users extending these APIs. If you want to change the generics, I'd prefer that all such generics changes in relation to all public APIs should get in 0.11.0 release in one shot.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/bloom/HoodieBloomIndex.java

yihua · 2022-01-31T18:53:52Z

...ent/hudi-client-common/src/main/java/org/apache/hudi/index/bloom/HoodieGlobalBloomIndex.java

            && !recordLocationHoodieKeyPair.get().getRight().getPartitionPath().equals(hoodieRecord.getPartitionPath())) {
          // Create an empty record to delete the record in the old partition
-          HoodieRecord<T> deleteRecord = new HoodieRecord(recordLocationHoodieKeyPair.get().getRight(),
+          HoodieRecord<R> deleteRecord = new HoodieAvroRecord(recordLocationHoodieKeyPair.get().getRight(),


This needs to be revisited to make it Avro agnostic later on.

yihua · 2022-01-31T19:05:55Z

...t/hudi-client-common/src/main/java/org/apache/hudi/index/simple/HoodieGlobalSimpleIndex.java

            if (config.getGlobalSimpleIndexUpdatePartitionPath() && !(inputRecord.getPartitionPath().equals(partitionPath))) {
              // Create an empty record to delete the record in the old partition
-              HoodieRecord<T> deleteRecord = new HoodieRecord(new HoodieKey(inputRecord.getRecordKey(), partitionPath), new EmptyHoodieRecordPayload());
+              HoodieRecord<R> deleteRecord = new HoodieAvroRecord(new HoodieKey(inputRecord.getRecordKey(), partitionPath), new EmptyHoodieRecordPayload());


Similar here for revisiting later on.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieAppendHandle.java

...ent/hudi-client-common/src/test/java/org/apache/hudi/testutils/HoodieWriteableTestTable.java

yihua · 2022-01-31T19:20:19Z

hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/index/FlinkHoodieIndex.java

  @PublicAPIMethod(maturity = ApiMaturityLevel.DEPRECATED)
  public abstract List<WriteStatus> updateLocation(List<WriteStatus> writeStatuses,
                                                   HoodieEngineContext context,
-                                                   HoodieTable<T, List<HoodieRecord<T>>, List<HoodieKey>, List<WriteStatus>> hoodieTable) throws HoodieIndexException;
+                                                   HoodieTable hoodieTable) throws HoodieIndexException;


Similar for engine-specific HoodieIndex classes to remove deprecated API methods altogether (in a separate PR).

...hudi-flink-client/src/test/java/org/apache/hudi/testutils/HoodieFlinkWriteableTestTable.java

hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/index/TestHoodieIndexConfigs.java

yihua · 2022-01-31T19:33:01Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieAvroRecord.java

+
+package org.apache.hudi.common.model;
+
+public class HoodieAvroRecord<T extends HoodieRecordPayload> extends HoodieRecord<T> {


As discussed, this is more of an intermediate solution for row writer, before RFC-46 revamps it completely.

hudi-bot · 2022-02-04T04:05:12Z

CI report:

e07767a Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

…che#3893) Co-authored-by: Raymond Xu <[email protected]>

yihua force-pushed the HUDI-2656-generalize-hoodie-index branch 3 times, most recently from b7989f4 to 47eaf38 Compare October 29, 2021 17:03

nsivabalan added the priority:blocker Production down; release blocker label Nov 3, 2021

xushiyan self-assigned this Nov 7, 2021

nsivabalan removed the priority:blocker Production down; release blocker label Nov 15, 2021

alexeykudinkin reviewed Dec 17, 2021

View reviewed changes

xushiyan force-pushed the HUDI-2656-generalize-hoodie-index branch 2 times, most recently from 8aff109 to af9fb3c Compare January 20, 2022 21:33

apache deleted a comment from hudi-bot Jan 20, 2022

xushiyan force-pushed the HUDI-2656-generalize-hoodie-index branch from af9fb3c to fd6f671 Compare January 20, 2022 23:55

apache deleted a comment from hudi-bot Jan 21, 2022

xushiyan force-pushed the HUDI-2656-generalize-hoodie-index branch from a9c7cf6 to ff7e9f7 Compare January 22, 2022 06:10

xushiyan changed the title ~~[WIP][HUDI-2656] Generalize HoodieIndex for flexible record data type~~ [HUDI-2656] Generalize HoodieIndex for flexible record data type Jan 22, 2022

xushiyan force-pushed the HUDI-2656-generalize-hoodie-index branch from ff7e9f7 to 70ef5f9 Compare January 22, 2022 19:01

xushiyan reviewed Jan 22, 2022

View reviewed changes

xushiyan force-pushed the HUDI-2656-generalize-hoodie-index branch from 70ef5f9 to b466262 Compare January 22, 2022 19:19

xushiyan reviewed Jan 22, 2022

View reviewed changes

yihua self-assigned this Jan 24, 2022

alexeykudinkin reviewed Jan 25, 2022

View reviewed changes

xushiyan mentioned this pull request Jan 28, 2022

[HUDI-3318] [RFC-46] Optimize Record Payload handling #4697

Merged

5 tasks

xushiyan force-pushed the HUDI-2656-generalize-hoodie-index branch 2 times, most recently from 455714a to de5b0b9 Compare January 31, 2022 17:35

alexeykudinkin reviewed Jan 31, 2022

View reviewed changes

alexeykudinkin approved these changes Jan 31, 2022

View reviewed changes

yihua commented Jan 31, 2022

View reviewed changes

yihua and others added 3 commits February 3, 2022 17:32

[HUDI-2656] Generalize HoodieIndex for flexible record data type

d431bef

address comments

b777870

Resolve build failure after rebase

e07767a

yihua force-pushed the HUDI-2656-generalize-hoodie-index branch from d7a82da to e07767a Compare February 4, 2022 01:44

apache deleted a comment from hudi-bot Feb 4, 2022

yihua merged commit b8601a9 into apache:master Feb 4, 2022

liusenhua pushed a commit to liusenhua/hudi that referenced this pull request Mar 1, 2022

[HUDI-2656] Generalize HoodieIndex for flexible record data type (apa…

878531c

…che#3893) Co-authored-by: Raymond Xu <[email protected]>

vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022

[HUDI-2656] Generalize HoodieIndex for flexible record data type (apa…

97a2683

…che#3893) Co-authored-by: Raymond Xu <[email protected]>


		package org.apache.hudi.common.model;

		public class HoodieAvroRecord<T extends HoodieRecordPayload> extends HoodieRecord<T> {

		public Map<String, Integer> mapFileWithInsertsToUniquePartition(JavaRDD<WriteStatus> writeStatusRDD) {
		Map<String, Integer> mapFileWithInsertsToUniquePartition(JavaRDD<WriteStatus> writeStatusRDD) {

		public Tuple2<Long, Integer> getHBasePutAccessParallelism(final JavaRDD<WriteStatus> writeStatusRDD) {
		Tuple2<Long, Integer> getHBasePutAccessParallelism(final JavaRDD<WriteStatus> writeStatusRDD) {

[HUDI-2656] Generalize HoodieIndex for flexible record data type #3893

[HUDI-2656] Generalize HoodieIndex for flexible record data type #3893

Uh oh!

Conversation

yihua commented Oct 29, 2021 • edited by xushiyan Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

yihua left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

yihua commented Oct 29, 2021 •

edited by xushiyan

Loading