HDDS-14053. Extract generic MinHeapMergeIterator from SstFileSetReader

[swamirishi]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

Suggested change

	@Override