You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@hudi.apache.org by "yihua (via GitHub)" <gi...@apache.org> on 2025/05/08 21:44:51 UTC

[PR] [HUDI-9397] Move index enabling condition to MetadataPartitionType [hudi]

yihua opened a new pull request, #13276:
URL: https://github.com/apache/hudi/pull/13276

   ### Change Logs
   
   _Describe context and summary for this change. Highlight if any code was copied._
   
   ### Impact
   
   _Describe any public API or user-facing feature change or any performance impact._
   
   ### Risk level (write none, low medium or high below)
   
   _If medium or high, explain what verification was done to mitigate the risks._
   
   ### 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](https://hudi.apache.org/contribute/developer-setup#website) to make
     changes to the website._
   
   ### Contributor's checklist
   
   - [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
   - [ ] Change Logs and Impact were stated clearly
   - [ ] Adequate tests were added if applicable
   - [ ] CI passed
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [HUDI-9397] Move index enabling condition to MetadataPartitionType [hudi]

Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.

nsivabalan commented on code in PR #13276:
URL: https://github.com/apache/hudi/pull/13276#discussion_r2080831783


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -251,7 +260,7 @@ public List<MetadataPartitionType> getEnabledPartitionTypes() {
   protected boolean initializeIfNeeded(HoodieTableMetaClient dataMetaClient,
                                        Option<String> inflightInstantTimestamp) throws IOException {
     HoodieTimer timer = HoodieTimer.start();
-    List<MetadataPartitionType> metadataPartitionsToInit = new ArrayList<>(MetadataPartitionType.getValidValues().length);
+    List<MetadataPartitionType> metadataPartitionsToInit = new ArrayList<>(MetadataPartitionType.values().length);

Review Comment:
   this might return 1 additional value compared to getValidValues(). 
   we ignore ALL_PARTITIONS in getValidValues(). 
    



##########
hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java:
##########
@@ -179,6 +180,12 @@ public void constructMetadataPayload(HoodieMetadataPayload payload, GenericRecor
     }
   },
   EXPRESSION_INDEX(PARTITION_NAME_EXPRESSION_INDEX_PREFIX, "expr-index-", -1) {
+    @Override
+    public boolean isMetadataPartitionSupported(HoodieTableMetaClient metaClient) {
+      // Partition stats is supported for partitioned tables only

Review Comment:
   this comment does not make sense here.



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -193,8 +194,16 @@ protected HoodieBackedTableMetadataWriter(StorageConfiguration<?> storageConf,
     ValidationUtils.checkArgument(!initialized || this.metadata != null, "MDT Reader should have been opened post initialization");
   }
 
+  @VisibleForTesting
   List<MetadataPartitionType> getEnabledPartitions(HoodieMetadataConfig metadataConfig, HoodieTableMetaClient metaClient) {
-    return MetadataPartitionType.getEnabledPartitions(metadataConfig, metaClient);
+    if (!metadataConfig.isEnabled()) {
+      return Collections.emptyList();
+    }
+    return MetadataPartitionType.getValidValues().stream()
+        .filter(partitionType -> partitionType.isMetadataPartitionSupported(metaClient)
+            && (partitionType.isMetadataPartitionEnabled(metadataConfig)
+            || partitionType.isMetadataPartitionAvailable(metaClient)))
+        .collect(Collectors.toList());

Review Comment:
   not sure if we can simplify. 
   each if meant to serve diff purpose. 
   a. isMetadataPartitionSupported -> some partitions are not supported in v6, but only version 8 and above. 
   b. isMetadataPartitionEnabled -> based on writer properties, we determine which partitions are enabled. 
   c. isMetadataPartitionAvailable -> For secondary and expression index, even if writer properties does not suggest to be enabled, if its already enabled and available, we wanted to update these partitions. Also, incase of async metadata partition initialization, even if current writer properties does not have a certain partition as enabled, if its part of inflight metadata partitions in table config, we wanted to update these partitions. 
   
   So, not sure how we can fold all these into one method. 



##########
hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java:
##########
@@ -200,6 +207,12 @@ public String getPartitionPath(HoodieTableMetaClient metaClient, String indexNam
     }
   },
   SECONDARY_INDEX(HoodieTableMetadataUtil.PARTITION_NAME_SECONDARY_INDEX_PREFIX, "secondary-index-", 7) {
+    @Override
+    public boolean isMetadataPartitionSupported(HoodieTableMetaClient metaClient) {
+      // Partition stats is supported for partitioned tables only

Review Comment:
   same here



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org