HDDS-14053. Extract generic MinHeapMergeIterator from SstFileSetReader #9409

swamirishi · 2025-12-01T23:57:01Z

What changes were proposed in this pull request?

In order to rewrite objectId mapping phase in snapshot diff we need tableMergeIterator to avoid randomized gets for each objectId and also removing the requirement for a separate objectIds table to track all the objectIds witnessed while iterating through the keyTable/fileTable/directoryTable.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14053

How was this patch tested?

Existing unit tests should be good since this is already used by SstFileSetReader

Copilot

Pull request overview

This PR extracts a generic MinHeapMergeIterator class from SstFileSetReader to enable its reuse in other parts of the codebase, specifically for rewriting the objectId mapping phase in snapshot diff operations. The refactoring generalizes the heap-based iterator merging logic while maintaining backward compatibility with existing SST file reading functionality.

Key changes:

Extracted generic MinHeapMergeIterator abstract class that merges multiple sorted iterators using a min-heap
Refactored SstFileSetReader.MultipleSstFileIterator to extend the new generic iterator
Added DBStore.getMergeIterator() method to merge multiple database tables

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
`MinHeapMergeIterator.java`	New generic abstract class providing min-heap based merging of multiple sorted iterators with resource management
`SstFileSetReader.java`	Refactored to use the new `MinHeapMergeIterator`, removing the old `HeapEntry` inner class
`DBStore.java`	Added default `getMergeIterator()` method to merge multiple tables into a single iterator
`ClosableIterator.java`	Changed from extending `AutoCloseable` to `Closeable` for more specific exception handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-04T01:14:21Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStore.java

+        for (int i = 0; i < tableValues.size(); i++) {
+          tableValues.set(i, keysToMerge.getOrDefault(i, defaultNullValue).getValue());
+        }
+        return newKeyValue(key, tableValues);


The tableValues list is reused and mutated on every call to merge(). This means all returned Collection<Object> values from the iterator share the same underlying list, and subsequent calls to next() will modify the contents of previously returned collections. This can lead to subtle bugs if callers retain references to these collections. Consider creating a new list for each merge result: return newKeyValue(key, new ArrayList<>(tableValues));

Suggested change

return newKeyValue(key, tableValues);

return newKeyValue(key, new java.util.ArrayList<>(tableValues));

Copilot · 2025-12-04T01:14:21Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+    assert minHeap.peek() != null;
+    // Get current key from heap
+    K currentKey = minHeap.peek().getCurrentKey();
+    // Clear the keys list by setting all entries to null.


The comment on line 113 says "Clear the keys list by setting all entries to null" but the code actually calls keys.clear() which removes all entries, not sets them to null. The comment should be updated to: "Clear the keys map from the previous iteration."

Suggested change

// Clear the keys list by setting all entries to null.

// Clear the keys map from the previous iteration.

Copilot · 2025-12-04T01:14:21Z

.../rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/SstFileSetReader.java

  /**
   * The MultipleSstFileIterator class is an abstract base for iterating over multiple SST files.
   * It uses a PriorityQueue to merge keys from all files in sorted order.
   * Each file's iterator is wrapped in a HeapEntryWithFileIdx object,
   * which ensures stable ordering for identical keys by considering the file index.
   * @param <T>


The comment still mentions "HeapEntryWithFileIdx" and "stable ordering for identical keys by considering the file index", but the current implementation no longer maintains stable ordering by file index. The HeapEntry class was moved to MinHeapMergeIterator and doesn't use file index for comparison. This comment should be updated to reflect the current behavior.

Copilot · 2025-12-04T01:14:22Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+ * using a min-heap. The {@code MinHeapMergeIterator} ensures the merged output is in sorted order
+ * by repeatedly polling the smallest element from the heap of iterators.
+ *
+ * @param <K> the type of keys being merged, must be {@link Comparable}


The class javadoc states that K "must be {@link Comparable}" but this is not accurate. The type parameter K doesn't have a Comparable bound, and the class accepts any type with a Comparator. The documentation should be updated to reflect that K needs a Comparator provided via the constructor.

Suggested change

* @param <K> the type of keys being merged, must be {@link Comparable}

* @param <K> the type of keys being merged; a {@link Comparator} for K must be provided via the constructor

Copilot · 2025-12-04T01:14:22Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStore.java

+   *         to a collection of values from the tables
+   */
+  default <KEY> ClosableIterator<KeyValue<KEY, Collection<Object>>> getMergeIterator(
+      Comparator<KEY> keyComparator, KEY prefix, Table<KEY, Object>... table) {


The varargs parameter table is not validated for null or empty. If an empty array or null is passed, this will cause issues: an empty array will create an iterator that never returns elements but won't fail gracefully, and null will cause a NullPointerException. Consider adding validation at the start of the method.

Suggested change

Comparator<KEY> keyComparator, KEY prefix, Table<KEY, Object>... table) {

Comparator<KEY> keyComparator, KEY prefix, Table<KEY, Object>... table) {

if (table == null || table.length == 0) {

throw new IllegalArgumentException("At least one table must be provided to getMergeIterator.");

}

Copilot · 2025-12-04T01:14:22Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+    IOException exception = null;
+    for (int idx = 0; idx < iterators.size(); idx++) {
+      try {
+        closeItrAtIndex(idx);
+      } catch (IOException e) {
+        exception = e;
+      }
+    }
+    if (exception != null) {
+      throw new UncheckedIOException(exception);


The close() method only preserves the last IOException that occurred during cleanup. If multiple iterators throw exceptions during close(), only the last one is thrown, potentially hiding other important errors. Consider collecting all exceptions or using a suppressed exception pattern to preserve all error information.

Suggested change

IOException exception = null;

for (int idx = 0; idx < iterators.size(); idx++) {

try {

closeItrAtIndex(idx);

} catch (IOException e) {

exception = e;

}

}

if (exception != null) {

throw new UncheckedIOException(exception);

IOException firstException = null;

for (int idx = 0; idx < iterators.size(); idx++) {

try {

closeItrAtIndex(idx);

} catch (IOException e) {

if (firstException == null) {

firstException = e;

} else {

firstException.addSuppressed(e);

}

}

}

if (firstException != null) {

throw new UncheckedIOException(firstException);

Copilot · 2025-12-04T01:14:22Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+  public void close() {
+    IOException exception = null;
+    for (int idx = 0; idx < iterators.size(); idx++) {
+      try {
+        closeItrAtIndex(idx);
+      } catch (IOException e) {
+        exception = e;
+      }
+    }
+    if (exception != null) {
+      throw new UncheckedIOException(exception);
+    }
+  }


The close() method doesn't clear the minHeap after closing all iterators. While the iterators themselves are closed and set to null in the list, HeapEntry objects remain in the minHeap, preventing proper garbage collection and potentially causing issues if the iterator is accidentally used after close().

Copilot · 2025-12-04T01:14:23Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStore.java

+    KeyValue<KEY, Object> defaultNullValue = newKeyValue(null, null);
+    Comparator<KeyValue<KEY, Object>> comparator = Comparator.comparing(KeyValue::getKey, keyComparator);
+    return new MinHeapMergeIterator<KeyValue<KEY, Object>, Table.KeyValueIterator<KEY, Object>,
+        KeyValue<KEY, Collection<Object>>>(table.length + 1, comparator) {


The constructor creates a PriorityQueue with size Math.max(numberOfIterators, 1) but then creates a MinHeapMergeIterator with table.length + 1 on line 202. This results in a heap size one larger than the number of tables, which is wasteful and inconsistent. The size should be table.length not table.length + 1.

Suggested change

KeyValue<KEY, Collection<Object>>>(table.length + 1, comparator) {

KeyValue<KEY, Collection<Object>>>(table.length, comparator) {

Copilot · 2025-12-04T01:14:23Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+      iterators.set(idx, null);
+    }
+  }
+


This method overrides ClosableIterator.close; it is advisable to add an Override annotation.

Suggested change

@Override

Change-Id: I2118dca4d36001dcb80be870e2ac0ff34228892c

jojochuang · 2025-12-10T22:26:01Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/util/ClosableIterator.java

 * An {@link Iterator} that may hold resources until it is closed.
 */
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {


This change is acceptable. Closeable is a subclass of AutoCloseable, and the only difference is it throws IOException instead of Exception. This is not a public API.

jojochuang · 2025-12-10T22:33:38Z

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/MinHeapMergeIterator.java

+
+  protected abstract I getIterator(int idx) throws RocksDBException, IOException;
+
+  private boolean initHeap() throws IOException, RocksDBException {


moved from SstFileSetRead.MultipleSstFileIterator.initMinHeap()

jojochuang · 2025-12-10T22:33:46Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+  /**
+   * A wrapper class that holds an iterator and its current value for heap operations.
+   */
+  private static final class HeapEntry<T> implements Comparable<HeapEntry<T>> {


this is moved from SstFileSetRead.HeapEntry, with index and comparator.
index is not used.
comparator is customizable.

jojochuang · 2025-12-10T22:34:32Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+   * A wrapper class that holds an iterator and its current value for heap operations.
+   */
+  private static final class HeapEntry<T> implements Comparable<HeapEntry<T>> {
+    private final int index;


index is not used.

ozone/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/MinHeapMergeIterator.java

Lines 117 to 121 in 9e3bd7f

HeapEntry<K> entry = minHeap.poll();

int idx = entry.index;

// Set the key for the current entry in the keys list.

keys.put(idx, entry.getCurrentKey());

if (entry.advance()) {

It is being used while setting the value in the Map returned

jojochuang · 2025-12-10T22:36:36Z

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/MinHeapMergeIterator.java

+
+  protected abstract I getIterator(int idx) throws RocksDBException, IOException;
+
+  private boolean initHeap() throws IOException, RocksDBException {


moved from SstFileSetReader.MultipleSstFileIterator.initMeanHeap()

jojochuang · 2025-12-10T22:40:35Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+  protected abstract V merge(Map<Integer, K> keysToMerge);
+
+  @Override
+  public V next() {


moved from SstFileSetReader.MultipleSstFileIterator.next(), except that it also manages keys map.

jojochuang · 2025-12-10T22:54:10Z

.../rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/SstFileSetReader.java

-      while (!minHeap.isEmpty()) {
-        minHeap.poll().close();
-      }
+    protected T merge(Map<Integer, T> keys) {


this is equivalent to the original code. Time complexity is O(1) constant.

jojochuang · 2025-12-10T22:57:57Z

...db-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java

+ * @param <I> the type of iterators being used, must extend {@link Iterator} and implement {@link Closeable}
+ * @param <V> the type of the final merged output
+ */
+public abstract class MinHeapMergeIterator<K, I extends Iterator<K> & Closeable, V>


Encapsulates several methods from SstFileSetReader.MultipleSstFileIterator, to be used by DBStore.

jojochuang · 2025-12-10T23:08:14Z

Except that index is not used, the rest is good.
I noticed the original MultipleSstFileIterator did not have test case. Do we plan to add test cases for MinHeapMergeIterator?

swamirishi · 2025-12-10T23:49:20Z

MultipleSstFileIterator

Yeah let me write one

Change-Id: Id6864055e3fd0c779d3fdf467555c5e260b07532

Change-Id: I9db0a28efbeefd91634cdb5dbf8122e9a385d598

Change-Id: I43b834786424069910a902452ab0eda10c90e7d1

jojochuang

lgtm

jojochuang · 2025-12-12T20:09:49Z

.../rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/SstFileSetReader.java

            fileReader.open(sstFile.toAbsolutePath().toString());
            estimatedSize += fileReader.getTableProperties().getNumEntries();
+          } catch (RocksDBException e) {
+            throw new RocksDatabaseException("Failed to open SST file: " + sstFile, e);


wrap RocksDBException with RocksDatabaseException because RocksDatabaseException is an IOException, which simplifies error handle of the caller

Change-Id: I109909334847096052445a4c9c263b798568cf55

swamirishi · 2025-12-12T23:30:53Z

Thank you @jojochuang for reviewing the patch

swamirishi requested review from jojochuang, sadanand48 and smengcl December 1, 2025 23:57

swamirishi added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Dec 1, 2025

adoroszlai changed the title ~~HDDS-14053. Make MinHeapIterator in SstFileSetReader more generic to work with any Autoclosable iterator~~ HDDS-14053. Extract generic MinHeapMergeIterator from SstFileSetReader Dec 2, 2025

swamirishi force-pushed the HDDS-14053 branch from 747b376 to ae7d88c Compare December 2, 2025 19:22

jojochuang requested a review from Copilot December 4, 2025 01:06

Copilot started reviewing on behalf of jojochuang December 4, 2025 01:07 View session

Copilot finished reviewing on behalf of jojochuang December 4, 2025 01:09

Copilot AI reviewed Dec 4, 2025

View reviewed changes

HDDS-14053. Extract generic MinHeapMergeIterator from SstFileSetReader

9e3bd7f

Change-Id: I2118dca4d36001dcb80be870e2ac0ff34228892c

swamirishi force-pushed the HDDS-14053 branch from 1c30074 to 9e3bd7f Compare December 4, 2025 22:07

jojochuang reviewed Dec 10, 2025

View reviewed changes

swamirishi added 3 commits December 12, 2025 12:27

Merge remote-tracking branch 'apache/master' into HEAD

df22f21

Change-Id: Id6864055e3fd0c779d3fdf467555c5e260b07532

HDDS-14053. Add test

655bbb0

Change-Id: I9db0a28efbeefd91634cdb5dbf8122e9a385d598

HDDS-14053. Fix exception handling

ac75395

Change-Id: I43b834786424069910a902452ab0eda10c90e7d1

jojochuang approved these changes Dec 12, 2025

View reviewed changes

swamirishi marked this pull request as ready for review December 12, 2025 21:57

HDDS-14053. Fix checkstyle

bddf1e3

Change-Id: I109909334847096052445a4c9c263b798568cf55

swamirishi merged commit ffb08af into apache:master Dec 12, 2025
43 checks passed

	return newKeyValue(key, tableValues);
	return newKeyValue(key, new java.util.ArrayList<>(tableValues));

	// Clear the keys list by setting all entries to null.
	// Clear the keys map from the previous iteration.

	* @param <K> the type of keys being merged, must be {@link Comparable}
	* @param <K> the type of keys being merged; a {@link Comparator} for K must be provided via the constructor

	KeyValue<KEY, Collection<Object>>>(table.length + 1, comparator) {
	KeyValue<KEY, Collection<Object>>>(table.length, comparator) {


		protected abstract I getIterator(int idx) throws RocksDBException, IOException;

		private boolean initHeap() throws IOException, RocksDBException {

	HeapEntry<K> entry = minHeap.poll();
	int idx = entry.index;
	// Set the key for the current entry in the keys list.
	keys.put(idx, entry.getCurrentKey());
	if (entry.advance()) {

HDDS-14053. Extract generic MinHeapMergeIterator from SstFileSetReader #9409

HDDS-14053. Extract generic MinHeapMergeIterator from SstFileSetReader #9409

Uh oh!

Conversation

swamirishi commented Dec 1, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

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!

jojochuang commented Dec 10, 2025

Uh oh!

swamirishi commented Dec 10, 2025

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swamirishi commented Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!