[I] Move secondary index merge logic to MetadataPartitionType.combineMetadataPayloads [hudi]

["hudi-bot (via GitHub)" <gi...@apache.org>]

hudi-bot opened a new issue, #17300:
URL: https://github.com/apache/hudi/issues/17300

   [https://github.com/apache/hudi/pull/12105/files#r1815875535]
   
   We need to move [this logic|https://github.com/apache/hudi/blob/a7512a206c5a1e8ce251cac7a302632a57d8c848/hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java#L855-L858] inside `HoodieMetadataPayload.combineSecondaryIndexRecord`, and need to override `MetadataPartitionType.combineMetadataPayloads` for secondary index.
   
   While updating secondary index, merging logic should follow same logic as readFromBaseAndMergeWithLogRecords API
   
   ## JIRA info
   
   - Link: https://issues.apache.org/jira/browse/HUDI-8436
   - Type: Sub-task
   - Parent: https://issues.apache.org/jira/browse/HUDI-9096
   - Fix version(s):
     - 1.1.0


-- 
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.apache.org

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

Re: [I] Move secondary index merge logic to MetadataPartitionType.combineMetadataPayloads [hudi]

["rangareddy (via GitHub)" <gi...@apache.org>]

rangareddy commented on issue #17300:
URL: https://github.com/apache/hudi/issues/17300#issuecomment-5365818376

   This issue was reviewed as part of the JIRA-migrated backlog triage (HUDI-8436).
   
   **Findings: half done - and the half that landed has no production caller. This is worth more attention than the title suggests.**
   
   **The method exists.** `hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:398`:
   
   ```java
   public static Option<HoodieRecord<HoodieMetadataPayload>> combineSecondaryIndexRecord(
   ```
   
   **But nothing calls it.** Searching production code for `combineSecondaryIndexRecord` finds only the definition itself - its sole other reference anywhere in the tree is a test, `hudi-hadoop-common/src/test/java/org/apache/hudi/metadata/TestHoodieMetadataPayload.java:323`.
   
   **And the wiring this ticket asks for was never added.** In `hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java`, `SECONDARY_INDEX` does **not** override `combineMetadataPayloads`. Other partition types do - `COLUMN_STATS` at `:263`, `ALL_PARTITIONS` at `:286`, and others - so secondary index falls through to the base implementation at `:418`:
   
   ```java
   /**
    * Merge old and new metadata payloads. By default, it returns the newer payload.
    * Implementations can override this method to merge the payloads depending on the partition type.
    */
   public HoodieMetadataPayload combineMetadataPayloads(HoodieMetadataPayload older, HoodieMetadataPayload newer) {
     return newer;
   }
   ```
   
   So secondary-index payload merging is last-writer-wins, not the delete-aware merge that `combineSecondaryIndexRecord` implements. Given the description says this merge "should follow same logic as `readFromBaseAndMergeWithLogRecords`", that difference looks like a correctness risk rather than tech debt - worth checking whether a stale or deleted secondary-index entry can win a merge on the write path.
   
   Suggest re-prioritising accordingly, and either wiring the existing method up or removing it so it stops reading as done.
   
   Keeping this open.
   


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