Copilot commented on code in PR #15823:
URL: https://github.com/apache/lucene/pull/15823#discussion_r2934590460
##########
lucene/core/src/java/org/apache/lucene/util/PriorityQueue.java:
##########
@@ -189,6 +189,18 @@ public final T add(T element) {
return heap[1];
}
+ /**
+ * Adds an Object to a PriorityQueue without upHeap. Note: only use it when
all elements have a
+ * same value.
+ */
+ public final void addNoShift(T element) {
+ // TODO: Check element equalsTo lastElement, if we can get the comparator.
Review Comment:
Typo/grammar in the new TODO comment: "equalsTo" should be "equal to".
##########
lucene/core/src/java/org/apache/lucene/util/PriorityQueue.java:
##########
@@ -189,6 +189,18 @@ public final T add(T element) {
return heap[1];
}
+ /**
+ * Adds an Object to a PriorityQueue without upHeap. Note: only use it when
all elements have a
+ * same value.
+ */
+ public final void addNoShift(T element) {
Review Comment:
New API addNoShift is not covered by tests. Since lucene/core already has
util-level tests in TestPriorityQueue, please add a focused test that builds a
queue via addNoShift under its intended precondition (all elements compare
equal), then mutates one element and calls updateTop()/pop() to verify heap
behavior remains correct.
##########
lucene/core/src/java/org/apache/lucene/util/PriorityQueue.java:
##########
@@ -189,6 +189,18 @@ public final T add(T element) {
return heap[1];
}
+ /**
+ * Adds an Object to a PriorityQueue without upHeap. Note: only use it when
all elements have a
Review Comment:
Grammar in the new Javadoc: "a same value" should be "the same value".
##########
lucene/core/src/java/org/apache/lucene/util/PriorityQueue.java:
##########
@@ -189,6 +189,18 @@ public final T add(T element) {
return heap[1];
}
+ /**
+ * Adds an Object to a PriorityQueue without upHeap. Note: only use it when
all elements have a
+ * same value.
+ */
+ public final void addNoShift(T element) {
+ // TODO: Check element equalsTo lastElement, if we can get the comparator.
+
+ int index = size + 1;
Review Comment:
addNoShift currently bypasses heap restoration without any guardrails, so an
accidental misuse can corrupt the heap and lead to incorrect top()/pop()
results later. Since PriorityQueue already has access to lessThan, please
either enforce/validate the precondition (at least via an assert that the new
element is not less than its parent / compares equal to existing elements) or
make this API less error-prone (e.g., restrict visibility if possible). Also,
consider removing the TODO by implementing the check directly.
##########
lucene/core/src/java/org/apache/lucene/util/PriorityQueue.java:
##########
@@ -189,6 +189,18 @@ public final T add(T element) {
return heap[1];
}
+ /**
+ * Adds an Object to a PriorityQueue without upHeap. Note: only use it when
all elements have a
+ * same value.
Review Comment:
The Javadoc precondition for addNoShift is ambiguous ("all elements have the
same value"—is that by comparator/lessThan ordering, by equals, or by some
key?). Since violating the heap invariant can silently break top()/pop(),
please reword the contract in terms of ordering (e.g., element must not compare
less than its parent / must compare equal to existing elements) and consider
documenting that callers must maintain the heap property.
--
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]
