[HUDI-9318] Refactor the log records presentation in FileGroupRecordB… #13213

cshuo · 2025-04-23T11:22:25Z

…uffer

Change Logs

Refactor the log records presentation in FileGroupRecordBuffer

cache ordering value for HoodieRecord to avoid dup calculating.
introduce a pojo BufferedRecord to substitute Pair<Option<T>, Map<String, Object>> for record buffer in file group reader.
convert the buffered record into binary-format before put into spillable map to save space, and reduce spilling.

Impact

Reduce heap size of record to make ExternalSpillableMap less prone to spill.

Risk level (write none, low medium or high below)

medium

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".

The config description must be updated if new configs are added or the default value of the configs are changed
Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the instruction to make
changes to the website.

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java

the-other-tim-brown · 2025-04-23T14:06:04Z

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java

+    return isDelete;
+  }
+
+  public boolean isDeleteRecordWithNaturalOrder() {


naturalOrdering is a confusing concept to me. usually natural ordering would be something like 1,2,3 but in this case 0 is greater than 1, 2, and 3. I think having some other naming would be helpful here like hardDelete or forcedDeletion

IIUC, isDeleteRecordWithNaturalOrder is checked for queries like DELETE FROM TABLE, if so, +1 for hardDelete. WDYT, cc @nsivabalan @yihua @danny0405

let's keep it as it is now.

yeah, lets not make too many changes in this patch.

cshuo · 2025-04-23T16:58:15Z

cc @nsivabalan @linliu-code @yihua

hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java

linliu-code · 2025-04-23T17:53:14Z

@cshuo , how much memory does it save?

the-other-tim-brown · 2025-04-23T18:06:09Z

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java

+   * @param record The engine row
+   * @return row with binary format
+   */
+  public abstract T toBinaryRow(T record);


Can you add some context for why this is required?

Instead of enforcing all implementations to have a concept of a Binary version, can we move this into the code that reads the data into format T? Or possible move it to the seal functionality?

We think through this and think should add a ne interface for 2 reasons:

seal is only for copy purposes, not care about the row format itself;

toBinaryRow will just focus on row format transformation.

Thus to avoid unnecessary copy or transf of rows.

We should only enforce the row that would be put in spillable map has binary format. And seal is called in multiple places, e.g., baseRecord read from base file is also sealed, where toBinary is not necessary to avoid additional costs. That's the context we introduce a new method toBinary to separate seal.

We can move this conversion directly to the code that converts from the avro to the row. This doesn't need to be part of the abstract class in my opinion.

I also think that the UnsafeRow conversion is Spark only and that can be done in the log record iterator where the Avro record is deserialized to UnsafeRow, without adding the public API method toBinaryRow to HoodieReaderContext. However I see there is difficulty due to schema evolution and other handling that require more refactoring to achieve the goal. If we want to keep toBinaryRow in this PR, mark it as @Deprecated or @PublicAPIMethod(maturity = ApiMaturityLevel.DEPRECATED) so further usage is disallowed.

Besides Spark unsafe row conversion, Flink also needs convert GenericRowData info BinaryRowData. And binary conversion is needed not just in log reading, it's also necessary after merging in fg reader, where the merged row may be not binary format either, so currently it seems necessary to make HoodieReaderContext capable of performing binary conversion.

We can also make this the responsibility of the merger though to return an optimal representation. If the merger returns an avro, we will run it through the same conversion code used when reading the log files

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java

nsivabalan

Can you fix PR description to all out what changes we are doing in this patch.

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java

danny0405 · 2025-04-23T23:31:30Z

hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java

@@ -89,6 +89,8 @@ public class HoodieSparkRecord extends HoodieRecord<InternalRow> {
   */
  private final transient StructType schema;

+  private Comparable<?> orderingValue;


should we make it transient here, not sure Kyro serialization will ignore this.

to confirm: Kyro would ignore the transient value

cshuo · 2025-04-23T23:59:05Z

Can you fix PR description to all out what changes we are doing in this patch.

updated

…uffer

yihua · 2025-04-24T07:53:34Z

...ient/hudi-spark-client/src/main/scala/org/apache/hudi/BaseSparkInternalRowReaderContext.java

+    if (internalRow instanceof UnsafeRow) {
+      return internalRow;
+    }
+    final UnsafeProjection unsafeProjection = HoodieInternalRowUtils.getCachedUnsafeProjection(schema);


Let's create a follow-up JIRA to see if we can further simplify this part by considering schema evolution cc @jonvex , without having to get the projection instance per row.

issure created: https://issues.apache.org/jira/browse/HUDI-9337

yihua · 2025-04-24T07:56:08Z

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java

-   * @param recordOption An option of record.
-   * @param metadataMap  A map containing the record metadata.
-   * @param schema       The Avro schema of the record.
+   * @param record An option of record.


So now this record is non-null, correct?

Suggested change

* @param record An option of record.

* @param record The record.

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java

yihua · 2025-04-24T08:24:29Z

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java

+   * @param record The engine row
+   * @return row with binary format
+   */
+  public abstract T toBinaryRow(T record);


I also think that the UnsafeRow conversion is Spark only and that can be done in the log record iterator where the Avro record is deserialized to UnsafeRow, without adding the public API method toBinaryRow to HoodieReaderContext. However I see there is difficulty due to schema evolution and other handling that require more refactoring to achieve the goal. If we want to keep toBinaryRow in this PR, mark it as @Deprecated or @PublicAPIMethod(maturity = ApiMaturityLevel.DEPRECATED) so further usage is disallowed.

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java

hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java

yihua · 2025-04-24T09:15:14Z

hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java

              }
            }
            Option<Pair<HoodieRecord, Schema>> mergedRecord = recordMerger.get().merge(
-                readerContext.constructHoodieRecord(older, olderInfoMap), readerContext.getSchemaFromMetadata(olderInfoMap),
-                readerContext.constructHoodieRecord(newer, newerInfoMap), readerContext.getSchemaFromMetadata(newerInfoMap), props);
+                readerContext.constructHoodieRecord(olderRecord), readerContext.getSchemaFromBufferRecord(olderRecord),


One thing to validate later is that custom merger implementation can return HoodieEmptyRecord to indicate deletes. That should be properly handled in the CUSTOM merge mode.

hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java

hudi-bot · 2025-04-24T11:33:22Z

CI report:

2474394 UNKNOWN
4239f8a UNKNOWN
3159946 UNKNOWN
226ce4d Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

danny0405 · 2025-04-24T13:05:45Z

+1, nice fix~

cshuo changed the title ~~[HUDI-9318] Refactor the log records presentation in FileGroupRecordB…~~ [WIP][HUDI-9318] Refactor the log records presentation in FileGroupRecordB… Apr 23, 2025

github-actions bot added the size:L PR with lines of changes in (300, 1000] label Apr 23, 2025

the-other-tim-brown reviewed Apr 23, 2025

View reviewed changes

cshuo force-pushed the HUDI-9318 branch 2 times, most recently from f9b68bf to 2474394 Compare April 23, 2025 16:54

cshuo changed the title ~~[WIP][HUDI-9318] Refactor the log records presentation in FileGroupRecordB…~~ [HUDI-9318] Refactor the log records presentation in FileGroupRecordB… Apr 23, 2025

linliu-code reviewed Apr 23, 2025

View reviewed changes

hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java Outdated Show resolved Hide resolved

the-other-tim-brown reviewed Apr 23, 2025

View reviewed changes

linliu-code reviewed Apr 23, 2025

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java Show resolved Hide resolved

nsivabalan reviewed Apr 23, 2025

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java Outdated Show resolved Hide resolved

danny0405 reviewed Apr 23, 2025

View reviewed changes

cshuo and others added 2 commits April 24, 2025 15:28

[HUDI-9318] Refactor the log records presentation in FileGroupRecordB…

333603a

…uffer

[MINOR] Fix flaky test testDeletePartitionsV2 (apache#13207)

87f00fe

cshuo force-pushed the HUDI-9318 branch from 65d8d3b to 87f00fe Compare April 24, 2025 07:30

yihua reviewed Apr 24, 2025

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java Outdated Show resolved Hide resolved

yihua reviewed Apr 24, 2025

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java Show resolved Hide resolved

hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java Outdated Show resolved Hide resolved

Address review comments

e2299ad

yihua reviewed Apr 24, 2025

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java Outdated Show resolved Hide resolved

fix comments

226ce4d

danny0405 approved these changes Apr 24, 2025

View reviewed changes

linliu-code approved these changes Apr 24, 2025

View reviewed changes

danny0405 merged commit 6566fca into apache:release-1.0.2 Apr 25, 2025
61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-9318] Refactor the log records presentation in FileGroupRecordB… #13213

[HUDI-9318] Refactor the log records presentation in FileGroupRecordB… #13213

cshuo commented Apr 23, 2025 •

edited

Loading

the-other-tim-brown Apr 23, 2025

cshuo Apr 23, 2025

danny0405 Apr 24, 2025

nsivabalan Apr 24, 2025

cshuo commented Apr 23, 2025

linliu-code commented Apr 23, 2025

the-other-tim-brown Apr 23, 2025

the-other-tim-brown Apr 23, 2025

danny0405 Apr 23, 2025

cshuo Apr 23, 2025

the-other-tim-brown Apr 24, 2025

yihua Apr 24, 2025

cshuo Apr 24, 2025

the-other-tim-brown Apr 24, 2025

nsivabalan left a comment

danny0405 Apr 23, 2025

danny0405 Apr 24, 2025

cshuo commented Apr 23, 2025

yihua Apr 24, 2025

danny0405 Apr 24, 2025

yihua Apr 24, 2025

yihua Apr 24, 2025

yihua Apr 24, 2025

hudi-bot commented Apr 24, 2025

danny0405 commented Apr 24, 2025

	* @param record An option of record.
	* @param record The record.

[HUDI-9318] Refactor the log records presentation in FileGroupRecordB… #13213

[HUDI-9318] Refactor the log records presentation in FileGroupRecordB… #13213

Conversation

cshuo commented Apr 23, 2025 • edited Loading

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

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

cshuo commented Apr 23, 2025

linliu-code commented Apr 23, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

cshuo commented Apr 23, 2025

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

hudi-bot commented Apr 24, 2025

CI report:

danny0405 commented Apr 24, 2025

cshuo commented Apr 23, 2025 •

edited

Loading