You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "gsmiller (via GitHub)" <gi...@apache.org> on 2024/11/01 16:33:50 UTC
[PR] A few small tweaks to VectorUtil#findNextGEQ [lucene]
gsmiller opened a new pull request, #13972:
URL: https://github.com/apache/lucene/pull/13972
1. Rearrange/rename the parameters to be more idiomatic (e.g., follow conventions of Arrays#... methods)
2. Add assert to ensure expected sortedness we may rely on in the future (so we're not trappy)
3. Migrate PostingsReader to call VectorUtil instead of VectorUtilSupport (so it benefits from the common assert)
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] A few small tweaks to VectorUtil#findNextGEQ [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on code in PR #13972:
URL: https://github.com/apache/lucene/pull/13972#discussion_r1826036633
##########
lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1755,13 +1755,13 @@ static long readVLong15(DataInput in) throws IOException {
}
}
- private static int findNextGEQ(long[] buffer, int length, long target, int from) {
- for (int i = from; i < length; ++i) {
+ private static int findNextGEQ(long[] buffer, long target, int from, int to) {
Review Comment:
Made this change for consistency but don't have any strong feelings about it. If there's a preference to just leave it be, I'm fine reverting this file.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] A few small tweaks to VectorUtil#findNextGEQ [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller merged PR #13972:
URL: https://github.com/apache/lucene/pull/13972
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] A few small tweaks to VectorUtil#findNextGEQ [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13972:
URL: https://github.com/apache/lucene/pull/13972#discussion_r1827331927
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene101/Lucene101PostingsReader.java:
##########
@@ -601,7 +599,7 @@ public int advance(int target) throws IOException {
}
}
- int next = VECTOR_SUPPORT.findNextGEQ(docBuffer, docBufferSize, target, docBufferUpto);
+ int next = VectorUtil.findNextGEQ(docBuffer, target, docBufferUpto, docBufferSize);
Review Comment:
I did it this way after doing something similar with PostingDecodingUtil, which didn't really belong to `VectorUtil`. I agree it's nicer the way you did it, I would not expect performance implications if inlining works properly, nightly benchmarks will tell us.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] A few small tweaks to VectorUtil#findNextGEQ [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on code in PR #13972:
URL: https://github.com/apache/lucene/pull/13972#discussion_r1826037886
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene101/Lucene101PostingsReader.java:
##########
@@ -601,7 +599,7 @@ public int advance(int target) throws IOException {
}
}
- int next = VECTOR_SUPPORT.findNextGEQ(docBuffer, docBufferSize, target, docBufferUpto);
+ int next = VectorUtil.findNextGEQ(docBuffer, target, docBufferUpto, docBufferSize);
Review Comment:
@jpountz I made this change so that everything goes through `VectorUtil` and the `assert`. It also seemed a little more consistent with how we leverage `VectorUtil` vs. `VectorUtilSupport` elsewhere, but I'm not sure if you had a good reason for writing it the way you did. If there's a performance (or other) consideration here, I'd be happy to understand that better.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org