You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/10/21 20:34:56 UTC
[GitHub] [solr] epugh opened a new pull request, #1107: SOLR-9775 fixed NPEs
epugh opened a new pull request, #1107:
URL: https://github.com/apache/solr/pull/1107
https://issues.apache.org/jira/browse/SOLR-9775
# Description
This is brought over from https://github.com/apache/lucene-solr/pull/116/files
# Solution
I feel like I am pulling a thread on a sweater..... It feels odd that we have so much handling around nulls.... And that this whole thing needs some refactoring so there aren't nulls.
In fact, there is kind of a comment about this in `QueryResultKey.unorderedCompare()`:
```
// SOLR-5618: if we had a guarantee that the lists never contained any duplicates,
// this logic could be a lot simpler
//
// (And of course: if the SolrIndexSearcher / QueryCommand was ever changed to
// sort the filter query list, then this whole method could be eliminated).
```
It feels like if we simplified the types of objects passed into the `QueryResultKey` this would all be simpler. It's literally just a key!
# Tests
Existing tests, and then added some to demonstrate the NPE and then the fix...
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1107:
URL: https://github.com/apache/solr/pull/1107#discussion_r1038341931
##########
solr/core/src/java/org/apache/solr/search/QueryResultKey.java:
##########
@@ -51,20 +52,28 @@ public QueryResultKey(
Query query, List<Query> filters, Sort sort, int nc_flags, int minExactCount) {
this.query = query;
this.sort = sort;
- this.filters = filters;
this.nc_flags = nc_flags;
this.minExactCount = minExactCount;
int h = query.hashCode();
- if (filters != null) {
- for (Query filt : filters)
+ if (filters == null) {
+ this.filters = null;
+ } else {
+ this.filters = filters.stream().filter(Objects::nonNull).collect(Collectors.toList());
+ for (Query filt : this.filters) {
Review Comment:
what about using stream instead of for loop?
> h += this.filters.stream().mapToInt(Query::hashCode).sum();
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1107:
URL: https://github.com/apache/solr/pull/1107#discussion_r1038361120
##########
solr/core/src/java/org/apache/solr/search/QueryResultKey.java:
##########
@@ -51,20 +52,28 @@ public QueryResultKey(
Query query, List<Query> filters, Sort sort, int nc_flags, int minExactCount) {
this.query = query;
this.sort = sort;
- this.filters = filters;
this.nc_flags = nc_flags;
this.minExactCount = minExactCount;
int h = query.hashCode();
- if (filters != null) {
- for (Query filt : filters)
+ if (filters == null) {
+ this.filters = null;
+ } else {
+ this.filters = filters.stream().filter(Objects::nonNull).collect(Collectors.toList());
Review Comment:
suggestion. what about using `Collectors.toUnmodifiableList()`, and same for the one below
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1290460862
@kagan770 I wanted to flag that this old PR is looking ready to get merged if you want to review ;-).
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1289705313
@risdenk that is sooo much better. I guess I don't have the courage of my convictions to change an array to a `List`!
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1107:
URL: https://github.com/apache/solr/pull/1107#discussion_r1038361030
##########
solr/core/src/java/org/apache/solr/search/QueryResultKey.java:
##########
@@ -33,16 +36,14 @@ public final class QueryResultKey implements Accountable {
final Query query;
final Sort sort;
- final SortField[] sfields;
+ final List<SortField> sfields;
Review Comment:
humm... @stillalex any way to demonstrate that in the test?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1287447435
Okay, look at my most recent tests... While they pass, I am not sure I am actually making things 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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1289207031
@epugh take a look at the commit bbda9ffe54d6b91274ea049b2ce8a0bdb101c42a I just pushed. See if you like the logic 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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1289705657
@cpoerschke thoughts on this PR? I think it's ready to merge.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1107:
URL: https://github.com/apache/solr/pull/1107#discussion_r1038473703
##########
solr/core/src/java/org/apache/solr/search/QueryResultKey.java:
##########
@@ -51,20 +52,28 @@ public QueryResultKey(
Query query, List<Query> filters, Sort sort, int nc_flags, int minExactCount) {
this.query = query;
this.sort = sort;
- this.filters = filters;
this.nc_flags = nc_flags;
this.minExactCount = minExactCount;
int h = query.hashCode();
- if (filters != null) {
- for (Query filt : filters)
+ if (filters == null) {
+ this.filters = null;
+ } else {
+ this.filters = filters.stream().filter(Objects::nonNull).collect(Collectors.toList());
+ for (Query filt : this.filters) {
Review Comment:
I'm open to it... While I like for loops, maybe cause I'm not up on stream, we already are doing that...
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1335542883
offering a review and a 'looks good to me' (even if non binding)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1492586901
@stillalex @risdenk I'm starting to look at various PR's that I've had open for a long time, and trying to get them merged or closed.
Are we still happy with this one?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1107: SOLR-9775 fixed NPEs
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1107:
URL: https://github.com/apache/solr/pull/1107#issuecomment-1492589453
@stillalex can you push up to this PR the changes you were thinking about?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #1107: SOLR-9775 fixed NPEs
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1107:
URL: https://github.com/apache/solr/pull/1107
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1107:
URL: https://github.com/apache/solr/pull/1107#discussion_r1038432311
##########
solr/core/src/java/org/apache/solr/search/QueryResultKey.java:
##########
@@ -33,16 +36,14 @@ public final class QueryResultKey implements Accountable {
final Query query;
final Sort sort;
- final SortField[] sfields;
+ final List<SortField> sfields;
Review Comment:
yeah, good question. I don't think it's an easy answer and I could not actually find any test verifying this. not sure about the lucene code (that's where this Accountable interface comes from), I don't have it handy atm.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1107: SOLR-9775 fixed NPEs
Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1107:
URL: https://github.com/apache/solr/pull/1107#discussion_r1038344272
##########
solr/core/src/java/org/apache/solr/search/QueryResultKey.java:
##########
@@ -33,16 +36,14 @@ public final class QueryResultKey implements Accountable {
final Query query;
final Sort sort;
- final SortField[] sfields;
+ final List<SortField> sfields;
Review Comment:
I think there is one missed aspect here regarding the ramBytesUsed value. it currently will account for SortField array header and sum of entries (line 77 below). changing to List would have to update that part too. (as a side note it is also not including 'sort' field)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org