gashutos commented on code in PR #12334:
URL: https://github.com/apache/lucene/pull/12334#discussion_r1211208875
##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -204,13 +200,21 @@ private void updateCompetitiveIterator() throws
IOException {
return;
}
if (reverse == false) {
- encodeBottom(maxValueAsBytes);
+ if (queueFull) { // bottom is avilable only when queue is full
+ maxValueAsBytes = maxValueAsBytes == null ? new byte[bytesCount] :
maxValueAsBytes;
Review Comment:
>>we also don't know if we'll ever need these arrays. If the queue never
fills, we could unnecessarily have allocated one of them.
if ```queueFull``` is ```false```, that mean we will be breaching this
condition ```|| (queueFull == false && topValueSet == false)) return;``` only
if its ```after``` query where ```topValueSet``` is set to ```true```. We dont
allocate unnecessary here, i.e we initiliaze min/max values only if we are to
call ```encodeTop``` or ```encodeBottom``` for that.
I gave explanation w.r.t code in current PR, but if you were talking in
context of existing code, yes, we are allocating unnecessary in case
```queueFull``` is always false.
>>I think we still have enough information upfront though to eagerly
allocate these like we do today? Is it just a question of being eager vs. lazy
with these?
There is a problem if we follow the same approach, check this code,
```
} else {
if (queueFull) { // bottom is avilable only when queue is full
minValueAsBytes = minValueAsBytes == null ? new byte[bytesCount] :
minValueAsBytes;
encodeBottom(minValueAsBytes);
}
if (topValueSet) {
maxValueAsBytes = maxValueAsBytes == null ? new byte[bytesCount] :
maxValueAsBytes;
encodeTop(maxValueAsBytes);
}
}
```
if ```queueFUll``` is always false & there is a ```topValueSet```, we will
have ```minValueAsBytes``` set as ```[0, 0, 0, 0, 0, 0, 0, 0, 0]``` instead
```null```. This will result in incorrect ```minValueAsBytes``` in further
calculations.
##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -204,13 +200,21 @@ private void updateCompetitiveIterator() throws
IOException {
return;
}
if (reverse == false) {
- encodeBottom(maxValueAsBytes);
+ if (queueFull) { // bottom is avilable only when queue is full
+ maxValueAsBytes = maxValueAsBytes == null ? new byte[bytesCount] :
maxValueAsBytes;
Review Comment:
Does that clarify ?
--
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-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
