Copilot commented on code in PR #8912:
URL: https://github.com/apache/ozone/pull/8912#discussion_r2325882538
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/SstFileSetReader.java:
##########
@@ -233,70 +236,149 @@ public String next() {
}
}
- private abstract static class MultipleSstFileIterator<T> implements
ClosableIterator<T> {
+ /**
+ * A wrapper class that holds an iterator and its current value for heap
operations.
+ */
+ private static class HeapEntryWithFileIdx<T extends Comparable<T>>
+ implements Comparable<HeapEntryWithFileIdx<T>> {
+ private final ClosableIterator<T> iterator;
+ private T currentKey;
+ // To ensure stable ordering for identical keys
+ private final int fileIndex;
+
+ HeapEntryWithFileIdx(ClosableIterator<T> iterator, int fileIndex) {
+ this.iterator = iterator;
+ this.fileIndex = fileIndex;
+ advance();
+ }
+
+ void close() {
+ iterator.close();
+ }
+
+ boolean advance() {
+ if (iterator.hasNext()) {
+ currentKey = iterator.next();
+ return true;
+ } else {
+ currentKey = null;
+ return false;
+ }
+ }
+
+ T getCurrentKey() {
+ return currentKey;
+ }
+
+ @Override
+ public int compareTo(@Nonnull HeapEntryWithFileIdx<T> other) {
+ if (this.currentKey == null && other.currentKey == null) {
+ return 0;
+ }
+ if (this.currentKey == null) {
+ return 1;
+ }
+ if (other.currentKey == null) {
+ return -1;
+ }
+
+ int result = this.currentKey.compareTo(other.currentKey);
+ if (result == 0) {
+ // For identical keys, prefer the one from the file with the higher
index
+ return Integer.compare(other.fileIndex, this.fileIndex);
+ }
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null || getClass() != obj.getClass()) {
+ return false;
+ }
+
+ HeapEntryWithFileIdx<T> other = (HeapEntryWithFileIdx<T>) obj;
Review Comment:
The cast is unsafe and could throw ClassCastException. Consider using
instanceof check before casting or implementing type-safe equals method.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/SstFileSetReader.java:
##########
@@ -233,70 +236,149 @@ public String next() {
}
}
- private abstract static class MultipleSstFileIterator<T> implements
ClosableIterator<T> {
+ /**
+ * A wrapper class that holds an iterator and its current value for heap
operations.
+ */
+ private static class HeapEntryWithFileIdx<T extends Comparable<T>>
+ implements Comparable<HeapEntryWithFileIdx<T>> {
+ private final ClosableIterator<T> iterator;
+ private T currentKey;
+ // To ensure stable ordering for identical keys
+ private final int fileIndex;
+
+ HeapEntryWithFileIdx(ClosableIterator<T> iterator, int fileIndex) {
+ this.iterator = iterator;
+ this.fileIndex = fileIndex;
+ advance();
+ }
+
+ void close() {
+ iterator.close();
+ }
+
+ boolean advance() {
+ if (iterator.hasNext()) {
+ currentKey = iterator.next();
+ return true;
+ } else {
+ currentKey = null;
+ return false;
+ }
+ }
+
+ T getCurrentKey() {
+ return currentKey;
+ }
+
+ @Override
+ public int compareTo(@Nonnull HeapEntryWithFileIdx<T> other) {
+ if (this.currentKey == null && other.currentKey == null) {
+ return 0;
+ }
+ if (this.currentKey == null) {
+ return 1;
+ }
+ if (other.currentKey == null) {
+ return -1;
+ }
+
+ int result = this.currentKey.compareTo(other.currentKey);
+ if (result == 0) {
+ // For identical keys, prefer the one from the file with the higher
index
+ return Integer.compare(other.fileIndex, this.fileIndex);
+ }
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null || getClass() != obj.getClass()) {
+ return false;
+ }
+
+ HeapEntryWithFileIdx<T> other = (HeapEntryWithFileIdx<T>) obj;
+ return compareTo(other) == 0;
+ }
Review Comment:
The equals implementation violates the equals-hashCode contract. Two objects
that are equal according to compareTo(other) == 0 may have different hash codes
because hashCode includes the iterator object reference, but equals only
compares the comparable result.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
