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 2021/03/15 14:20:46 UTC
[GitHub] [solr] bruno-roustant opened a new pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
bruno-roustant opened a new pull request #18:
URL: https://github.com/apache/solr/pull/18
Adapts Solr to Lucene changes where SortedDocValues no longer extends BinaryDocValues (LUCENE-9796).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] madrob commented on pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #18:
URL: https://github.com/apache/solr/pull/18#issuecomment-799505762
> I propose that you remove the comment "//TODO: SortedDocValues quick fix" everywhere that you put it. In two places, I suggested alternative comments scoped to the feature of a possible future optimization. In the other places, I think the ord to BytesRef resolution can't be delayed/avoided.
I disagree with this. Maybe you're right, but we can leave the comment in until we've had more time to think this through.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] bruno-roustant commented on pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #18:
URL: https://github.com/apache/solr/pull/18#issuecomment-799553517
I'll take into account the comments. At the moment I'm fixing more classes as tests break.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] mikemccand commented on pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #18:
URL: https://github.com/apache/solr/pull/18#issuecomment-799687139
Maybe try to get a minimal PR pushed, first, to un-block Solr build, and then follow-on PR to better optimize things? Ahh it looks like we've already done just that (first step), great!
Thank you @bruno-roustant! This shows how important it is to get Solr consuming Lucene snapshot that we periodically choose to carefully upgrade.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #18:
URL: https://github.com/apache/solr/pull/18#discussion_r594385042
##########
File path: solr/core/src/java/org/apache/solr/uninverting/FieldCacheImpl.java
##########
@@ -1076,10 +1076,6 @@ public BinaryDocValues getTerms(LeafReader reader, String field) throws IOExcept
public BinaryDocValues getTerms(LeafReader reader, String field, float acceptableOverheadRatio) throws IOException {
BinaryDocValues valuesIn = reader.getBinaryDocValues(field);
- if (valuesIn == null) {
Review comment:
I'm removing this part. It seems to me the SortedDocValues are returned by getTermsIndex() so we don't need this block.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #18:
URL: https://github.com/apache/solr/pull/18#discussion_r594394186
##########
File path: solr/core/src/java/org/apache/solr/search/HashQParserPlugin.java
##########
@@ -112,7 +112,7 @@ public LongValues getValues(LeafReaderContext ctx, DoubleValues scores) throws I
@Override
public boolean advanceExact(int doc) throws IOException { atDoc = values.advanceExact(doc); return true; }
@Override
- public long longValue() throws IOException { return atDoc ? values.binaryValue().hashCode() : 0; }
+ public long longValue() throws IOException { return atDoc ? values.lookupOrd(values.ordValue()).hashCode() : 0; }//TODO: SortedDocValues quick fix
Review comment:
```suggestion
public long longValue() throws IOException { return atDoc ? values.lookupOrd(values.ordValue()).hashCode() : 0; }//TODO maybe cache hashCode if same ord as prev doc to save lookupOrd?
```
##########
File path: solr/core/src/java/org/apache/solr/search/join/HashRangeQuery.java
##########
@@ -104,7 +104,7 @@ public float matchCost() {
}
private int hash(SortedDocValues docValues) throws IOException {
- BytesRef bytesRef = docValues.binaryValue();
+ BytesRef bytesRef = docValues.lookupOrd(docValues.ordValue());//TODO: SortedDocValues quick fix
Review comment:
```suggestion
//TODO maybe cache hashCode if same ord as prev doc to save lookupOrd?
BytesRef bytesRef = docValues.lookupOrd(docValues.ordValue());
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] dsmiley commented on pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #18:
URL: https://github.com/apache/solr/pull/18#issuecomment-799588251
> I disagree with this. Maybe you're right, but we can leave the comment in until we've had more time to think this through.
Then let's discuss now. I reviewed each one of them to hypothesize if it's plausible there is an optimization opportunity. For two, I thought maybe, so I proposed more specific code comments than those general comments. Let's not commit "quick fix" comments; let's be more specific. In the other places, I felt the code was already doing what it can -- that there didn't seem to be a plausible way to avoid the BytesRef lookup. Do you disagree?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] uschindler commented on pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #18:
URL: https://github.com/apache/solr/pull/18#issuecomment-799609181
I agree, some of them are completely useless to comment on or fix them: E.g. SolrDocumentFetcher is just emulating stored fields, on every call to get a virtual stored field it gets a new docvalues instance and calls advanceExact. There's no need to optimize anything here. Other places are similar, like the ChildDocTransformer. And the tests I would not care.
There is room for opportunities and improvments at the other places where you sequentially read many docvalues one by one (e.g. in queries).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [solr] bruno-roustant closed pull request #18: SOLR-15261: SortedDocValues no longer extends BinaryDocValues
Posted by GitBox <gi...@apache.org>.
bruno-roustant closed pull request #18:
URL: https://github.com/apache/solr/pull/18
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org