[HUDI-8648] Fix a bug for secondary index deletion

[linliu-code]

Change Logs

When we try to search for the SI records for a given record key, the way we search for the secondary key could cause some SI records missed, and further cause some SI records are not deleted correctly.

Impact

Correct SI behavior.

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

[nsivabalan]

should we also do

deletedRecordsFromLogs.remove(recordKey);

within the if block.
just incase a record is deleted first and added back later. we should not treat it as deleted.

[linliu-code]

No, we cannot. The single usage of deletedRecordsFromLogs is to filter records from base file, .e.g., we have br, lr1(deleted), lr2(non-deleted). When we handle lr2, we cannot remove its rk, since rk is used to filter br.

[codope]

I think it's ok because the log records are handled in temporal order so it won't be treated as deleted.

[nsivabalan]

do we have test for this scenario ?
i.e.

add a record to logfile1, delete in logfile2 and then add it back in logfile3.
if the test works, I am good.

[nsivabalan]

why we are not merging the records from base to log records? I understand in case of secondary index, a record is either created or deleted and there is no real merge, but lets keep the original code as is and just add fixes on top of that.

[nsivabalan]

For eg, I went to master and made edits as below. all tests you have added in this patch worked.

+  @VisibleForTesting
+  public static Map<String, String> reverseLookupSecondaryKeysInternal(List<String> keySet, Map<String, HoodieRecord<HoodieMetadataPayload>> baseFileRecords,
+                                                                HoodieMetadataLogRecordReader logRecordScanner) {
+    Set<String> deletedRecordsFromLogs = new HashSet<>();
+    // Map of recordKey (primaryKey) -> log record that is not deleted for all input recordKeys
+    Map<String, HoodieRecord<HoodieMetadataPayload>> logRecordsMap = new HashMap<>();
+    logRecordScanner.getRecords().forEach(record -> {
+      String recordKey = SecondaryIndexKeyUtils.getRecordKeyFromSecondaryIndexKey(record.getRecordKey());
+      HoodieMetadataPayload payload = record.getData();
+      if (!payload.isDeleted()) { // process only valid records.
+        if (keySet.contains(recordKey)) {
+          logRecordsMap.put(recordKey, record);
+          deletedRecordsFromLogs.remove(recordKey); // we can check if its present and then remove if need be
+        }
+      } else {
+        deletedRecordsFromLogs.add(recordKey);
+        logRecordsMap.remove(recordKey);
+      }
+    });
+
+    Map<String, String> recordKeyMap = new HashMap<>();
+    if (baseFileRecords == null || baseFileRecords.isEmpty()) {
+      logRecordsMap.forEach((key1, value1) -> {
+        if (!value1.getData().isDeleted() && !deletedRecordsFromLogs.contains(key1)) {
+          recordKeyMap.put(key1, SecondaryIndexKeyUtils.getSecondaryKeyFromSecondaryIndexKey(value1.getRecordKey()));
+        }
+      });
+    } else {
+      // Iterate over all provided log-records, merging them into existing records
+      logRecordsMap.forEach((key1, value1) -> baseFileRecords.merge(key1, value1, (oldRecord, newRecord) -> {
+        Option<HoodieRecord<HoodieMetadataPayload>> mergedRecord = HoodieMetadataPayload.combineSecondaryIndexRecord(oldRecord, newRecord);
+        return mergedRecord.orElse(null);
+      }));
+      baseFileRecords.forEach((key, value) -> {
+        if (!deletedRecordsFromLogs.contains(key)) {
+          recordKeyMap.put(key, SecondaryIndexKeyUtils.getSecondaryKeyFromSecondaryIndexKey(value.getRecordKey()));
+        }
+      });
+    }
     return recordKeyMap;
   }

[linliu-code]

I can use the above code; but it is more complex than needed.

[linliu-code]

@nsivabalan , the reason is that we discuss here is that the logic of this function is too complex to understand. so we should try to make it as simple as possible.

[codope]

Both are fine IMO, but maybe I am biased due to prior work. Ideally, I would like to retain the semantics of combineSecondaryIndexRecord but the current patch is also simple to understand. Plus, if we filter out tombstone records before calling combineSecondaryIndexRecord then there is no point in keeping it. Please remove this method.

[linliu-code]

Yeah, I will adopt Siva's approach to avoid back-and-forth arguments.

[nsivabalan]

minor feedback

[codope]

Do we need to create a TreeSet of recordKeys? I don't think order of record keys matters here. I believe it was there already, probably to optimize contains check. But, even a simple HashSet would do right?

[linliu-code]

Yeah, the order of the record key here does not matter. I can check which one is more performant regarding the search.

[linliu-code]

Generally HashSet is faster than TreeSet; will change it.

[codope]

Both are fine IMO, but maybe I am biased due to prior work. Ideally, I would like to retain the semantics of combineSecondaryIndexRecord but the current patch is also simple to understand. Plus, if we filter out tombstone records before calling combineSecondaryIndexRecord then there is no point in keeping it. Please remove this method.

[codope]

@linliu-code After refactoring - Test Secondary Index With Updates Compaction Clustering Deletes *** FAILED ***
https://github.com/apache/hudi/actions/runs/12380514412/job/34557088618?pr=12447#step:5:8624

Maybe refactoring missed the actual fix which you had originally. Could you run the test locally multiple times to ensure flakiness is completely fixed?

[linliu-code]

Yeah, it may trigger the same bug before or a different bug. Will check.

[linliu-code]

@codope After the refactoring, the test failed quickly. Should be a regression. Let me change back to my previous version to confirm.

[hudi-bot]

CI report:

• b71c47f UNKNOWN
• f34959a UNKNOWN
• 3ccf599 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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