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

codope · 2022-02-18T13:28:04Z

What is the purpose of the pull request

Rework of #4761
This diff introduces follinwg 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.

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.

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

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

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java

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

nsivabalan · 2022-02-19T00:43:37Z

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

   * @param isMetaIndexColumnStatsForAllColumns - Is column stats indexing enabled for all columns
   */
-  private static List<String> getLatestColumns(HoodieTableMetaClient datasetMetaClient, boolean isMetaIndexColumnStatsForAllColumns) {
+  private static List<String> getColumnsToIndex(HoodieTableMetaClient datasetMetaClient, boolean isMetaIndexColumnStatsForAllColumns) {


a comment about L 834. I feel we can't directly take in RecordKeyFieldProp as is. may not work for all key gens.
may be we have to split with "," and then set the columns to index.
Can you think if there are any other places where we have this dependency and check if we have done the right thing

fixed here. But, there are couple of other places.. i'll create a separate patch.

sure. do we have a tracking jira?

I did not create a separate jira.. This is already being tracked in https://issues.apache.org/jira/browse/HUDI-3411

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

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

alexeykudinkin · 2022-02-24T02:15:02Z

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

-      return HoodieMetadataPayload.createBloomFilterMetadataRecord(
-          deleteFileInfo.getLeft(), deleteFileInfo.getRight(), instantTime, ByteBuffer.allocate(0), true);
-    }, 1).stream().collect(Collectors.toList());
+    HoodieData<Pair<String, String>> deleteFileListRDD = engineContext.parallelize(deleteFileList,


Do we really need to cast this action t/h RDD? Do we envision that this will scale past the point when we won't be able to handle this on the driver?

I'm worried about serialization cost we incur for every record we handle t/h RDD (serializing/de closure) to be able to create a single object

alexeykudinkin · 2022-02-24T02:16:37Z

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

-    }, 1).stream().collect(Collectors.toList());
+    HoodieData<Pair<String, String>> deleteFileListRDD = engineContext.parallelize(deleteFileList,
+        Math.max(deleteFileList.size(), recordsGenerationParams.getBloomIndexParallelism()));
+    return deleteFileListRDD.map(deleteFileInfo -> HoodieMetadataPayload.createBloomFilterMetadataRecord(


Let's create common override for this method (it seems to be used in 3 more places at least)

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

alexeykudinkin · 2022-02-24T02:29:25Z

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

+    return recordsStats;
+  }
+
+  public static class RecordsStats<T> implements Serializable {


What do we need this wrapper for?

Wrapper abstracts away the underlying metadata. I think write stat should be aware that it saves the record stats but not necessarily what those stats are composed of. Are you concerned about serde cost here? It shouldn't add much overhead over keeping it as a prvate field.

@codope i'm concerned about it as an abstraction that isn't bringing much value, while increasing complexity: It adds cognitive load to understand what it does for anybody interacting with it.

In general, i'd suggest to follow the principle to keep things as simple as possible, but no simpler than needed to solve the problem. It helps on many fronts:

Makes the code easier to comprehend

Makes component evolution easier (the simpler things are, the easier it is to evolve them)

Makes component age better: if things change and we need to refactor it -- the simpler the system is, the easier the refactoring will be

nsivabalan

Apart from addressing feedback from me and Alexey, was there any additional changes. I did not review entire patch, but did pointed review for the feedback given and addressed. Let me know if you have made any other additional changes. or point me to commits hashes that I need to look into specifically.

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

nsivabalan · 2022-03-08T02:01:36Z

@codope : am good with the patch. Can you rebase w/ latest master. we can land once CI is green. sorry, lets get this landed by tomorrow.

…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. [HUDI-3142] Metadata index initialization for bloom filters and column 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. [HUDI-1492] Metadata column stats index - handling delta writes - 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 Make HoodieColumnRangeMetadata serializable Add a config for colstat parallelism and address review Fix a UT and address review comments Union will fail if we choose min parallelism which could be 0 Fix payload construction Added HoodieIndex UTs based on apache#4516 Minor refactoring to address review comments Change the logic of building column range metadata Improve the way column stats is collected for avro records Take min index parallelism Fix conversion to long

hudi-bot · 2022-03-08T05:05:43Z

CI report:

fa193b7 Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

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

nsivabalan

LGTM. thanks for the perseverance. Definitely at a much better place right now.

alexeykudinkin · 2022-03-08T20:44:53Z

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

+    return recordsStats;
+  }
+
+  public static class RecordsStats<T> implements Serializable {


@codope i'm concerned about it as an abstraction that isn't bringing much value, while increasing complexity: It adds cognitive load to understand what it does for anybody interacting with it.

In general, i'd suggest to follow the principle to keep things as simple as possible, but no simpler than needed to solve the problem. It helps on many fronts:

Makes the code easier to comprehend

Makes component evolution easier (the simpler things are, the easier it is to evolve them)

Makes component age better: if things change and we need to refactor it -- the simpler the system is, the easier the refactoring will be

alexeykudinkin · 2022-03-08T22:19:31Z

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

+ */
+public class MetadataRecordsGenerationParams implements Serializable {
+
+  private final HoodieTableMetaClient dataMetaClient;


Let's limit the scope of this component to just parameters for Index Generation. Otherwise this has a potential to become a dependency magnet, where random dependencies will be added here to avoid threading them through.

BTW, i see it as Serializable, how are we serializing the metaClient?

alexeykudinkin · 2022-03-08T22:24:10Z

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

  }

+  private MetadataRecordsGenerationParams getRecordsGenerationParams() {
+    return new MetadataRecordsGenerationParams(


BTW, why do we even need this component if we can just get all of this from the Writer Config?

alexeykudinkin · 2022-03-08T22:26:27Z

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


+    if (config.isMetadataIndexColumnStatsForAllColumnsEnabled()) {
+      Map<String, HoodieColumnRangeMetadata<Comparable>> columnRangeMap = stat.getRecordsStats().isPresent()
+          ? stat.getRecordsStats().get().getStats() : new HashMap<>();


@codope that's what i was referring to with my comments regarding increased complexity in respect to RecordStats. Why not just have stat.getRecordsStats().get() instead?

Now, when reading this code reader actually need to understand what is this additional getStats() call is about and why it's needed, while w/o it the call-site is crystal clear and doesn't require scanning through of getRecordStats to understand what's going on

alexeykudinkin · 2022-03-08T22:31:41Z

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

+      Map<String, Map<String, Object>> columnToStats = new HashMap<>();
+      writeSchemaWithMetaFields.getFields().forEach(field -> columnToStats.putIfAbsent(field.name(), new HashMap<>()));
+      // collect stats for columns at once per record and keep iterating through every record to eventually find col stats for all fields.
+      recordList.forEach(record -> aggregateColumnStats(record, writeSchemaWithMetaFields, columnToStats, config.isConsistentLogicalTimestampEnabled()));


Can we, instead of placing iteration and aggregation into separate methods, consolidate them in aggregateColumnStats so that its signature actually is:

Map<String, Map<...>> aggregateColumnStats(records, writeSchema, ...)

alexeykudinkin · 2022-03-08T22:37:58Z

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

+   * @param columnRangeMap - old column range statistics, which will be merged in this computation
+   * @param columnToStats  - map of column to map of each stat and its value
+   */
+  public static void accumulateColumnRanges(Schema.Field field, String filePath,


Can we unify both of these methods into one?

alexeykudinkin · 2022-03-08T22:41:44Z

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

-  public static List<HoodieRecord> convertMetadataToBloomFilterRecords(HoodieCleanMetadata cleanMetadata,
-                                                                       HoodieEngineContext engineContext,
-                                                                       String instantTime) {
+  public static HoodieData<HoodieRecord> convertMetadataToBloomFilterRecords(HoodieCleanMetadata cleanMetadata,


nit: There's general convention that "context" objects are usually passed as first arg

Just FYI, no need to fix this

alexeykudinkin · 2022-03-08T22:45:17Z

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

    if (filePathWithPartition.endsWith(HoodieFileFormat.PARQUET.getFileExtension())) {
      List<HoodieColumnRangeMetadata<Comparable>> columnRangeMetadataList = new ArrayList<>();
      final Path fullFilePath = new Path(datasetMetaClient.getBasePath(), filePathWithPartition);
      if (!isDeleted) {


Deleted files handling is invariant of the file format, right?

alexeykudinkin · 2022-03-08T22:53:30Z

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

+        }
+        // set the max value of the field
+        if (fieldVal.compareTo(String.valueOf(columnStats.getOrDefault(MAX, ""))) > 0) {
+          columnStats.put(MAX, fieldVal);


We don't need Map for that, right? Let's instead create mutable object with all the statistics that we're collecting:

class FileColumnStats { Object min, max; long count, totalSize; // ... }

alexeykudinkin · 2022-03-08T23:05:43Z

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

-    return getLatestColumns(datasetMetaClient, false);
+  public static HoodieMetadataColumnStats mergeColumnStats(HoodieMetadataColumnStats oldColumnStats, HoodieMetadataColumnStats newColumnStats) {
+    ValidationUtils.checkArgument(oldColumnStats.getFileName().equals(newColumnStats.getFileName()));
+    if (newColumnStats.getIsDeleted()) {


We need to handle inverse case as well -- when existing records is a deleted one, otherwise we will merge incorrectly

…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]>

nsivabalan requested changes Feb 19, 2022

View reviewed changes

nsivabalan reviewed Feb 19, 2022

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieAppendHandle.java Outdated Show resolved Hide resolved

codope force-pushed the feature/HUDI-1492-deltawrite-record-stats-1 branch from 63aac43 to 97f253e Compare February 22, 2022 02:28

nsivabalan added the big-needle-movers label Feb 22, 2022

nsivabalan self-assigned this Feb 22, 2022

codope force-pushed the feature/HUDI-1492-deltawrite-record-stats-1 branch 2 times, most recently from 19ba560 to 125d2cd Compare February 23, 2022 06:22

codope mentioned this pull request Feb 23, 2022

[HUDI-1295][HUDI-3181] Enabling metadata table based index by default for tests #4516

Closed

5 tasks

codope assigned yihua Feb 23, 2022

alexeykudinkin requested changes Feb 24, 2022

View reviewed changes

codope changed the title ~~[HUDI-3356][HUDI-3203] HoodieData for metadata index records, bloom and colstats init~~ [HUDI-3258] HoodieData for metadata index records, bloom and colstats init Feb 28, 2022

nsivabalan reviewed Mar 3, 2022

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java Outdated Show resolved Hide resolved

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java Outdated Show resolved Hide resolved

codope force-pushed the feature/HUDI-1492-deltawrite-record-stats-1 branch 2 times, most recently from 21dc93b to ff1f746 Compare March 7, 2022 06:36

codope force-pushed the feature/HUDI-1492-deltawrite-record-stats-1 branch from ff1f746 to fa193b7 Compare March 8, 2022 03:35

nsivabalan approved these changes Mar 8, 2022

View reviewed changes

nsivabalan merged commit 575bc63 into apache:master Mar 8, 2022

alexeykudinkin reviewed Mar 8, 2022

View reviewed changes

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

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

Uh oh!

Conversation

codope commented Feb 18, 2022

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

nsivabalan commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hudi-bot commented Mar 8, 2022

CI report:

Uh oh!

nsivabalan left a comment

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 commented Mar 8, 2022 •

edited

Loading