You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/11/16 02:11:02 UTC
[GitHub] [lucene] rmuir opened a new pull request, #11938: fix overflows in compound assignments
rmuir opened a new pull request, #11938:
URL: https://github.com/apache/lucene/pull/11938
Fixes found from #11932 review of narrowing-compound-assignments
The checker is noisy, so it isn't being enabled here. Just fixing the rare gems that look reasonable.
Feel free to push commits here if you find stuff in that log file.
Closes #11932
--
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
[GitHub] [lucene] rmuir commented on pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #11938:
URL: https://github.com/apache/lucene/pull/11938#issuecomment-1317237989
Thanks @jpountz for going thru the file with me!
--
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
[GitHub] [lucene] rmuir merged pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
rmuir merged PR #11938:
URL: https://github.com/apache/lucene/pull/11938
--
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
[GitHub] [lucene] jpountz commented on a diff in pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
jpountz commented on code in PR #11938:
URL: https://github.com/apache/lucene/pull/11938#discussion_r1023992252
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java:
##########
@@ -108,7 +108,7 @@ public int nextDoc() throws IOException {
protected static class MergedVectorValues extends VectorValues {
private final List<VectorValuesSub> subs;
private final DocIDMerger<VectorValuesSub> docIdMerger;
- private final int cost;
+ private final long cost;
Review Comment:
I pushed a commit with what I had in mind. Essentially `cost()` and `size()` are the same thing on vectors except that the latter is expected to be accurate. Maybe we should rethink this, e.g. could `cost()` be required to be accurate on `VectorValues` instances, or could the base class make `cost()` final and delegate to `size()`?
--
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
[GitHub] [lucene] jpountz commented on a diff in pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
jpountz commented on code in PR #11938:
URL: https://github.com/apache/lucene/pull/11938#discussion_r1023705104
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java:
##########
@@ -108,7 +108,7 @@ public int nextDoc() throws IOException {
protected static class MergedVectorValues extends VectorValues {
private final List<VectorValuesSub> subs;
private final DocIDMerger<VectorValuesSub> docIdMerger;
- private final int cost;
+ private final long cost;
Review Comment:
Maybe we should remove `cost` altogether here. It's redundant with `size` on vectors?
--
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
[GitHub] [lucene] rmuir commented on a diff in pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11938:
URL: https://github.com/apache/lucene/pull/11938#discussion_r1024068862
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java:
##########
@@ -108,7 +108,7 @@ public int nextDoc() throws IOException {
protected static class MergedVectorValues extends VectorValues {
private final List<VectorValuesSub> subs;
private final DocIDMerger<VectorValuesSub> docIdMerger;
- private final int cost;
+ private final long cost;
Review Comment:
i'm happy either way, i'm not familiar enough with the specifics of the code, and I lean conservative when trying to fix issues before a release. we don't even have to touch it here now if there's no real risk of overflow (e.g. its just count of subs).
--
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
[GitHub] [lucene] rmuir commented on a diff in pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11938:
URL: https://github.com/apache/lucene/pull/11938#discussion_r1023927808
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java:
##########
@@ -108,7 +108,7 @@ public int nextDoc() throws IOException {
protected static class MergedVectorValues extends VectorValues {
private final List<VectorValuesSub> subs;
private final DocIDMerger<VectorValuesSub> docIdMerger;
- private final int cost;
+ private final long cost;
Review Comment:
i didn't look at how it was used, only doing shallow cleanups here. it is exposed via `long cost()` method, so it never made sense to be an int. what would `cost()` return instead?
--
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
[GitHub] [lucene] jpountz commented on pull request #11938: fix overflows in compound assignments
Posted by GitBox <gi...@apache.org>.
jpountz commented on PR #11938:
URL: https://github.com/apache/lucene/pull/11938#issuecomment-1316638147
I fixed one other warning from PointRangeQuery which looked worth addressing.
--
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