You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2026/01/01 16:48:38 UTC
[PR] HDDS-14330. MinHeapMergeIterator should use key comparator while popp… [ozone]
swamirishi opened a new pull request, #9576:
URL: https://github.com/apache/ozone/pull/9576
…ing out entries from the heap
Change-Id: Ia196692c74237ae100180b27dc5c2b5b661ba783
## What changes were proposed in this pull request?
MinHeapMergeIterator should use key comparator while popping out entries from the heap. Currently it uses Object.equals() which can fetch wrong results when comparing the key without the key comparator.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14330
## How was this patch tested?
Updated unit tests to use byte array
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14330. MinHeapMergeIterator should use key comparator while popping out entries from the heap [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9576:
URL: https://github.com/apache/ozone/pull/9576#discussion_r2656645676
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStore.java:
##########
@@ -198,7 +198,7 @@ default <KEY> ClosableIterator<KeyValue<KEY, Collection<Object>>> getMergeIterat
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) {
Review Comment:
I don't recall why it was table.length + 1 before.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/hadoop/hdds/utils/db/TestMinHeapMergeIterator.java:
##########
@@ -38,14 +40,15 @@
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Set;
+import org.apache.hadoop.hdds.StringUtils;
import org.junit.jupiter.api.Test;
/**
* Unit tests for {@link MinHeapMergeIterator}.
*/
class TestMinHeapMergeIterator {
- private static final Comparator<String> STRING_COMPARATOR = String::compareTo;
+ private static final Comparator<byte[]> BYTE_COMPARATOR = UnsignedBytes.lexicographicalComparator();
Review Comment:
is updating String to byte array a necessary change to reproduce the problem?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14330. MinHeapMergeIterator should use key comparator while popping out entries from the heap [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #9576:
URL: https://github.com/apache/ozone/pull/9576#discussion_r2662106437
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java:
##########
@@ -109,7 +109,7 @@ public V next() {
// Clear the keys list by setting all entries to null.
keys.clear();
// Advance all entries with the same key (from different files)
- while (!minHeap.isEmpty() && Objects.equals(minHeap.peek().getCurrentKey(), currentKey)) {
+ while (!minHeap.isEmpty() && comparator.compare(minHeap.peek().getCurrentKey(), currentKey) == 0) {
Review Comment:
cmiiw but if I understand correctly, when `compareTo()` and `equals()` are implemented correctly in parity, this change should not make much difference? except in the case where one argument could be null?
https://github.com/apache/ozone/blob/90994bb75f771f2023f9697430678ff05e3cc957/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/MinHeapMergeIterator.java#L183-L200
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14330. MinHeapMergeIterator should use key comparator while popping out entries from the heap [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9576:
URL: https://github.com/apache/ozone/pull/9576#discussion_r2656793929
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/hadoop/hdds/utils/db/TestMinHeapMergeIterator.java:
##########
@@ -38,14 +40,15 @@
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Set;
+import org.apache.hadoop.hdds.StringUtils;
import org.junit.jupiter.api.Test;
/**
* Unit tests for {@link MinHeapMergeIterator}.
*/
class TestMinHeapMergeIterator {
- private static final Comparator<String> STRING_COMPARATOR = String::compareTo;
+ private static final Comparator<byte[]> BYTE_COMPARATOR = UnsignedBytes.lexicographicalComparator();
Review Comment:
The iterator need not be always an Itr<String> it can be Itr<KeyValue<String, >> and KeyValue doesn't implement equals method
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStore.java:
##########
@@ -198,7 +198,7 @@ default <KEY> ClosableIterator<KeyValue<KEY, Collection<Object>>> getMergeIterat
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) {
Review Comment:
It was a typo
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14330. MinHeapMergeIterator should use key comparator while popping out entries from the heap [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi merged PR #9576:
URL: https://github.com/apache/ozone/pull/9576
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14330. MinHeapMergeIterator should use key comparator while popping out entries from the heap [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9576:
URL: https://github.com/apache/ozone/pull/9576#issuecomment-3705385786
Thank you @jojochuang for reviewing the patch
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org