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/18 15:39:27 UTC
[GitHub] [lucene] gsmiller opened a new pull request, #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
gsmiller opened a new pull request, #11950:
URL: https://github.com/apache/lucene/pull/11950
### Description
This fixes a bug where variants of `BinaryRangeFieldRangeQuery` will result in an NPE if the field doesn't exist in a segment.
--
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] gsmiller commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029494212
##########
lucene/CHANGES.txt:
##########
@@ -143,6 +143,9 @@ Bug Fixes
* GITHUB#11907: Fix latent casting bugs in BKDWriter. (Ben Trent)
+* GITHUB#11950: Fix NPE in BinaryRangeFieldRangeQuery variants when the queried field doesn't exist
+ in a segment or is of the wrong type. (Greg Miller)
Review Comment:
So I checked this scenario, and indexing with the wrong type does in fact result in an NPE. `reader.getBinaryDocValues` returns `null` if the field exists in the segment but wasn't indexed with BDV. In my test case, I'm indexing a `StringField` called "foo" and then trying to load "foo" through `reader.getBinaryDocValues`, which just returns `null`.
--
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 #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
jpountz commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029501693
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
+ return null;
+ }
+ BinaryDocValues binaryDocValues = DocValues.getBinary(reader, field);
Review Comment:
Oh sorry, I was confused and thought that `LeafReader#getBinaryDocValues` already performed the type check!
--
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] gsmiller commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029458836
##########
lucene/core/src/test/org/apache/lucene/search/TestRangeFieldsDocValuesQuery.java:
##########
@@ -226,4 +228,30 @@ public void testToString() {
Query q4 = LongRangeDocValuesField.newSlowIntersectsQuery("foo", longMin, longMax);
assertEquals("foo:[[101, 124, 137] TO [138, 145, 156]]", q4.toString());
}
+
+ public void testNoData() throws IOException {
+ Directory dir = newDirectory();
Review Comment:
Yeah, that's a nice pattern and a good suggestion. I'd prefer to be consistent though with the approach taken by the other unit tests in this class, and would rather not increase the scope of this bug fix to include changing the resource management in all these unit tests.
--
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] gsmiller merged pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller merged PR #11950:
URL: https://github.com/apache/lucene/pull/11950
--
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] gsmiller commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029471303
##########
lucene/CHANGES.txt:
##########
@@ -143,6 +143,9 @@ Bug Fixes
* GITHUB#11907: Fix latent casting bugs in BKDWriter. (Ben Trent)
+* GITHUB#11950: Fix NPE in BinaryRangeFieldRangeQuery variants when the queried field doesn't exist
+ in a segment or is of the wrong type. (Greg Miller)
Review Comment:
Let me test. I don't think I actually ran that part of the unit test prior to applying the fix. I'll make sure the changes entry is accurate.
--
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] gsmiller commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029481972
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
+ return null;
+ }
+ BinaryDocValues binaryDocValues = DocValues.getBinary(reader, field);
Review Comment:
Actually, I suppose it depends if we want to throw an `IllegalArgumentException` if the field has been indexed but as a different type. `DocValues#getBinary` will do the type checking and throw. If we go with your suggestion, we'll just silently ignore the field (treating it the same as if it doesn't exist in the segment).
I'd prefer we do the type checking. I think it makes more sense for users, and it's also consistent with other similar code paths. For example, `SortedNumericDocValuesField#newSlowRangeQuery` is a similar use-case, and relies on the factory methods for type checking.
--
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] shubhamvishu commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
shubhamvishu commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1028378099
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
Review Comment:
Maybe just `if (reader.getFieldInfos().fieldInfo(field) == null)` is fine as info is not used anywhere else than this null check.
--
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] shubhamvishu commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
shubhamvishu commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1028378099
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
Review Comment:
Maybe just `if (reader.getFieldInfos().fieldInfo(field) == null)` is fine as info is not used anywhere else than this null check?
--
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 #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
jpountz commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029197613
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
+ return null;
+ }
+ BinaryDocValues binaryDocValues = DocValues.getBinary(reader, field);
Review Comment:
I'm not sure I understand why we need to retrieve field infos, `getBinaryDocValues` already returns `null` when the field doesn't exist, so doing the following should be enough?
```
BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
if (binaryDocValues == null) {
return null;
}
return new BinaryRangeDocValues(binaryDocValues, numDims, numBytesPerDimension);
```
##########
lucene/CHANGES.txt:
##########
@@ -143,6 +143,9 @@ Bug Fixes
* GITHUB#11907: Fix latent casting bugs in BKDWriter. (Ben Trent)
+* GITHUB#11950: Fix NPE in BinaryRangeFieldRangeQuery variants when the queried field doesn't exist
+ in a segment or is of the wrong type. (Greg Miller)
Review Comment:
Is is true that there would be a NPE when the field is of the wrong type? I would have expected `CodecReader#getBinaryDocValues` to catch 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@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] gsmiller commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029470218
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
+ return null;
+ }
+ BinaryDocValues binaryDocValues = DocValues.getBinary(reader, field);
Review Comment:
Right, that's a better approach. Thanks for the suggestion. I work on a codebase that forbids loading doc-values directly from a `LeafReader` (enforcing going through `DocValues`) as a safeguard to dealing with the null references returned by a reader. So I'm in a habit of using the `DocValues` factory methods (and checking `FieldInfo` if _really_ needing to see if the field exists in a segment). I'll simplify this.
--
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] gsmiller commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1029466354
##########
lucene/core/src/java/org/apache/lucene/document/BinaryRangeFieldRangeQuery.java:
##########
@@ -91,7 +92,11 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private BinaryRangeDocValues getValues(LeafReader reader, String field) throws IOException {
- BinaryDocValues binaryDocValues = reader.getBinaryDocValues(field);
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info == null) {
Review Comment:
Sure, I'll simplify. Thanks!
--
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] shubhamvishu commented on a diff in pull request #11950: Fix NPE in BinaryRangeFieldRangeQuery when field does not exist or is of wrong type
Posted by GitBox <gi...@apache.org>.
shubhamvishu commented on code in PR #11950:
URL: https://github.com/apache/lucene/pull/11950#discussion_r1028376885
##########
lucene/core/src/test/org/apache/lucene/search/TestRangeFieldsDocValuesQuery.java:
##########
@@ -226,4 +228,30 @@ public void testToString() {
Query q4 = LongRangeDocValuesField.newSlowIntersectsQuery("foo", longMin, longMax);
assertEquals("foo:[[101, 124, 137] TO [138, 145, 156]]", q4.toString());
}
+
+ public void testNoData() throws IOException {
+ Directory dir = newDirectory();
Review Comment:
How about using try-with-resources here?
--
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