[HUDI-3356][HUDI-3142][HUDI-1492] Metadata column stats index - handling delta writes #4761

manojpec · 2022-02-07T19:08:46Z

What is the purpose of the pull request

Delta writes can change column ranges and the column stats index need
to be properly updated with new ranges to be consistent with the table
dataset. This fix add column stats index update support for the delta
writes.

Brief change log

This PR is stacked on top of #4746
Commit to review: 974c5ec

Index initialization and the metadata conversion now call the metadata table util
to get the column range stats for the delta writes.

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.

codope

@manojpec I think we can close #4740 #4746 and address comments from those PR here itself. Easier to review and land. Please do check my comments on the other two PRs as well.

hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java

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

codope · 2022-02-10T06:05:31Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java

+          return newColumnStats;
+        }
+        return new HoodieMetadataColumnStats(
+            newColumnStats.getFileName(),


So this field is called fileName in HoodieMetadataColumnStats but filePath in HoodieColumnRangeMetadata. If possible, can we keep the names consistent?

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

codope · 2022-02-10T06:27:38Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java

+                .stream().filter(Objects::nonNull).min(Comparator.naturalOrder()).orElse(null),
+            Arrays.asList(oldColumnStats.getMinValue(), newColumnStats.getMinValue())
+                .stream().filter(Objects::nonNull).max(Comparator.naturalOrder()).orElse(null),
+            oldColumnStats.getNullCount() + newColumnStats.getNullCount(),


Is my understanding correct that since this is append handle and we don't expect duplicates so simply add these stats?

…lter construction from index based on the type param - Write stats are converted to metadata index records during the commit. Making them use the HoodieData<HoodieRecord> type so that the record generation scales up with needs. - When building the BloomFilter from the index records, using the type param stored in the payload instead of hardcoded type.

…n stats partitions - When metadata table is created and when it decides to the initialization from the filesystem for the user dataset, all the enabled partitions need to be initialized along with FILES partition. This fix adds init support for bloom filter and column stats partitions.

- Delta writes can change column ranges and the column stats index need to be properly updated with new ranges to be consistent with the table dataset. This fix add column stats index update support for the delta writes.

nsivabalan · 2022-02-11T20:23:21Z

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

      updateWriteStatus(stat, result);
    }

+    Map<String, HoodieColumnRangeMetadata<Comparable>> columnRangeMap = stat.getRecordsStats().isPresent()


we should populate this only if metadata table and meta index is enabled right? or did we decide to serialize this info irrespective of it. bcoz, computing these stats will def add to write latency. So, trying to see if we can avoid if not required at all.

nsivabalan · 2022-02-11T20:24:48Z

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


+  public static final BiFunction<HoodieColumnRangeMetadata<Comparable>, HoodieColumnRangeMetadata<Comparable>, HoodieColumnRangeMetadata<Comparable>> COLUMN_RANGE_MERGE_FUNCTION =
+      (oldColumnRange, newColumnRange) -> {
+        ValidationUtils.checkArgument(oldColumnRange.getColumnName().equals(newColumnRange.getColumnName()));


do we need to validate for every record? can we move validations one level up and avoid this may be. since this is called or every record in an iterative manner, trying to see if this is over kill

nsivabalan · 2022-02-11T20:30:16Z

hudi-common/src/main/java/org/apache/hudi/metadata/MetadataRecordsGenerationParams.java

+import java.io.Serializable;
+import java.util.List;
+
+public class MetadataRecordsGenerationParams implements Serializable {


nsivabalan · 2022-02-11T20:40:43Z

...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java

+  private MetadataRecordsGenerationParams getRecordsGenerationParams() {
+    return new MetadataRecordsGenerationParams(
+        dataMetaClient, enabledPartitionTypes, dataWriteConfig.getBloomFilterType(),
+        dataWriteConfig.getBloomIndexParallelism(),


I feel we can't use the data table's bloom index parallelism here. may be we can re-use file listing parallelism or introduce new config.

nsivabalan · 2022-02-11T20:47:49Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

          if (fileBloomFilter == null) {
            LOG.error("Failed to read bloom filter for " + appendedFilePath);
-            return;
+            return Stream.empty();


shouldn't we be throwing exception here? how an a base file don't have bloom filter?

nsivabalan · 2022-02-11T20:49:02Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

+    final List<Pair<String, Map<String, Long>>> partitionToAppendedFilesList = partitionToAppendedFiles.entrySet()
+        .stream().map(entry -> Pair.of(entry.getKey(), entry.getValue())).collect(Collectors.toList());
+    final HoodieData<Pair<String, Map<String, Long>>> partitionToAppendedFilesRDD = engineContext.parallelize(partitionToAppendedFilesList,
+        Math.max(partitionToAppendedFiles.size(), recordsGenerationParams.getBloomIndexParallelism()));


why are we using bloom index parallelism in col stats generation ?

Added a config for col stats parallelism.

nsivabalan · 2022-02-11T20:54:34Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

-          }
-          return Stream.empty();
-        }, 1).stream().collect(Collectors.toList());
+    final List<String> columnsToIndex = getColumnsToIndex(recordsGenerationParams.getDataMetaClient());


sorry, with getLatestColumns(), I see 2nd arg as isMetaIndexColumnStatsForAllColumns. shouldn't we be setting it appropriately ? why letting it be false here?

nsivabalan · 2022-02-11T20:58:19Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

    processRestoreMetadata(metadataTableTimeline, restoreMetadata,
        partitionToAppendedFiles, partitionToDeletedFiles, lastSyncTs);

    final HoodieData<HoodieRecord> filesPartitionRecordsRDD = engineContext.parallelize(


not sure why FILES is here and BLOOM and col stats are in getMetadataPartitionTypeHoodieDataMap. can we also move FILES record generation to getMetadataPartitionTypeHoodieDataMap.

nsivabalan · 2022-02-11T20:59:36Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

-    }
-
-    return partitionToRecordsMap;
+    return getMetadataPartitionTypeHoodieDataMap(engineContext, recordsGenerationParams, instantTime,


may be I see the reason why. we can have a callback and call into that so that its consistent.

nsivabalan · 2022-02-11T21:02:46Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

+    if (writeStat instanceof HoodieDeltaWriteStat && ((HoodieDeltaWriteStat) writeStat).getRecordsStats().isPresent()) {
+      columnRangeMap = Option.of(((HoodieDeltaWriteStat) writeStat).getRecordsStats().get().getStats());
+    }
+    return getColumnStats(writeStat.getPartitionPath(), writeStat.getPath(), datasetMetaClient, columnsToIndex,


I don't see much value in passing columnRangeMap to getColumnStats. might as well do

List<HoodieColumnRangeMetadata<Comparable>> columnRangeMetadataList = new ArrayList<>(columnRangeMap.get().values()); return HoodieMetadataPayload.createColumnStatsRecords(partitionPath, columnRangeMetadataList, isDeleted);

here within if block and call getColumnStats() only in else block.

nsivabalan

I see some new code being added, but no new tests. For eg, AppendHandle we have new code added for which there aren't any tests only. Can you try to add some UTs.

hudi-bot · 2022-02-16T12:58:16Z

CI report:

82c87f6 Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

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

codope · 2022-02-18T13:29:12Z

Closing it in favor of #4848

…lter construction from index based on the type param (#4848) Rework of #4761 This diff introduces following changes: - Write stats are converted to metadata index records during the commit. Making them use the HoodieData type so that the record generation scales up with needs. - Metadata index init support for bloom filter and column stats partitions. - When building the BloomFilter from the index records, using the type param stored in the payload instead of hardcoded type. - Delta writes can change column ranges and the column stats index need to be properly updated with new ranges to be consistent with the table dataset. This fix add column stats index update support for the delta writes. Co-authored-by: Manoj Govindassamy <[email protected]>

…lter construction from index based on the type param (apache#4848) Rework of apache#4761 This diff introduces following changes: - Write stats are converted to metadata index records during the commit. Making them use the HoodieData type so that the record generation scales up with needs. - Metadata index init support for bloom filter and column stats partitions. - When building the BloomFilter from the index records, using the type param stored in the payload instead of hardcoded type. - Delta writes can change column ranges and the column stats index need to be properly updated with new ranges to be consistent with the table dataset. This fix add column stats index update support for the delta writes. Co-authored-by: Manoj Govindassamy <[email protected]>

manojpec force-pushed the feature/HUDI-1492-deltawrite-record-stats-1 branch from 6f056e6 to 974c5ec Compare February 7, 2022 19:23

codope reviewed Feb 10, 2022

View reviewed changes

manojpec and others added 5 commits February 11, 2022 20:11

[HUDI-1492] Metadata column stats index - handling delta writes

4c555d6

- Delta writes can change column ranges and the column stats index need to be properly updated with new ranges to be consistent with the table dataset. This fix add column stats index update support for the delta writes.

Fix a test and few minor refactorings

2c5d158

Make HoodieColumnRangeMetadata serializable

b0350aa

codope force-pushed the feature/HUDI-1492-deltawrite-record-stats-1 branch from 6d40a75 to b0350aa Compare February 11, 2022 14:51

nsivabalan requested changes Feb 11, 2022

View reviewed changes

nsivabalan reviewed Feb 11, 2022

View reviewed changes

nsivabalan self-assigned this Feb 16, 2022

Add a config for colstat parallelism and address review

82c87f6

codope mentioned this pull request Feb 18, 2022

[HUDI-3258] HoodieData for metadata index records, bloom and colstats init #4848

Merged

5 tasks

codope closed this Feb 18, 2022

[HUDI-3356][HUDI-3142][HUDI-1492] Metadata column stats index - handling delta writes #4761

[HUDI-3356][HUDI-3142][HUDI-1492] Metadata column stats index - handling delta writes #4761

Uh oh!

Conversation

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

Uh oh!

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

codope left a comment • 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!

Uh oh!

Uh oh!

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!

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!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

hudi-bot commented Feb 16, 2022

CI report:

Uh oh!

codope commented Feb 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

manojpec commented Feb 7, 2022 •

edited

Loading

codope left a comment •

edited

Loading