Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
dsmiley merged PR #3380: URL: https://github.com/apache/solr/pull/3380 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
dsmiley commented on PR #3380: URL: https://github.com/apache/solr/pull/3380#issuecomment-2945417091 I mispoke/misunderstood... thinking this would work for a `fl=aDvField` when no, that's handled by `SolrDocumentFetcher` `org.apache.solr.search.SolrDocumentFetcher#decorateDocValueFields`. That was somewhat recently optimized by @magibney (see it's use of `DocValuesIteratorCache`). I can update this PR CHANGES.txt to say "function queries in 'fl' param". -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
yurkor commented on PR #3380: URL: https://github.com/apache/solr/pull/3380#issuecomment-2945085396 @dsmiley Done. Will keep in mind - no force pushes) Usually it even worse - with single commit. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
dsmiley commented on PR #3380: URL: https://github.com/apache/solr/pull/3380#issuecomment-2944125812 OMG I'm slipping... you already have a CHANGES.txt. Nonetheless my feedback shows a massive difference between the actual code change (what you wrote about there) and how users understand/experience the optimization. Only a deep diving Solr hacker would know of ValueSourceAugmenter. An end-user has no clue what that means. **The audience of CHANGES.txt is users of Solr** -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
dsmiley commented on PR #3380:
URL: https://github.com/apache/solr/pull/3380#issuecomment-2944112030
Great!
(BTW, stop force-pushing to PRs. It resets the GH review state, therefore,
as a reviewer, I can't as easily clearly see your changes from one change to
the next).
CHANGES.txt: In 9.9, in the optimizations section, and perhaps this simple
sentence:
> Speed up returning fields ("fl" param) with DocValues referenced directly
or indirectly via function queries. (your name 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
dsmiley commented on code in PR #3380:
URL: https://github.com/apache/solr/pull/3380#discussion_r2127833365
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -61,32 +67,66 @@ public void setContext(ResultContext context) {
readerContexts = searcher.getIndexReader().leaves();
fcontext = ValueSource.newContext(searcher);
this.valueSource.createWeight(fcontext, searcher);
+ final var docList = context.getDocList();
+ if (docList == null) {
+return;
+ }
+
+ final int prefetchSize = Math.min(docList.size(), maxPrefetchSize);
+ final int[] ids = new int[prefetchSize];
+ int i = 0;
+ var iter = docList.iterator();
+ while (iter.hasNext() && i < prefetchSize) {
+ids[i++] = iter.nextDoc();
+ }
+ Arrays.sort(ids);
+ scores = new IntObjectHashMap<>(ids.length);
Review Comment:
Why is this called "scores"? I suggest a name like "cachedValuesById".
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -61,32 +67,66 @@ public void setContext(ResultContext context) {
readerContexts = searcher.getIndexReader().leaves();
fcontext = ValueSource.newContext(searcher);
this.valueSource.createWeight(fcontext, searcher);
+ final var docList = context.getDocList();
+ if (docList == null) {
+return;
+ }
+
+ final int prefetchSize = Math.min(docList.size(), maxPrefetchSize);
+ final int[] ids = new int[prefetchSize];
+ int i = 0;
+ var iter = docList.iterator();
+ while (iter.hasNext() && i < prefetchSize) {
+ids[i++] = iter.nextDoc();
+ }
+ Arrays.sort(ids);
+ scores = new IntObjectHashMap<>(ids.length);
+
+ FunctionValues values = null;
+ int docBase = -1;
+ int currentIdx = -1;
+ for (int docid : ids) {
+int idx = ReaderUtil.subIndex(docid, readerContexts);
+if (currentIdx != idx) {
+ currentIdx = idx;
+ LeafReaderContext rcontext = readerContexts.get(idx);
+ docBase = rcontext.docBase;
+ values = valueSource.getValues(fcontext, rcontext);
+}
+int localId = docid - docBase;
+var value = values.objectVal(localId);
+scores.put(docid, value != null ? value : NULL_SENTINEL);
+ }
} catch (IOException e) {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "exception for valuesource " +
valueSource, e);
}
}
Map fcontext;
SolrIndexSearcher searcher;
List readerContexts;
+ IntObjectHashMap scores;
@Override
- public void transform(SolrDocument doc, int docid, DocIterationInfo docInfo)
{
-// This is only good for random-access functions
-
-try {
-
- // TODO: calculate this stuff just once across diff functions
- int idx = ReaderUtil.subIndex(docid, readerContexts);
- LeafReaderContext rcontext = readerContexts.get(idx);
- FunctionValues values = valueSource.getValues(fcontext, rcontext);
- int localId = docid - rcontext.docBase;
- setValue(doc, values.objectVal(localId));
-} catch (IOException e) {
- throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR,
- "exception at docid " + docid + " for valuesource " + valueSource,
- e);
+ public void transform(SolrDocument doc, int docid, DocIterationInfo
docIterationInfo) {
+Object scoreValue = (scores != null) ? scores.get(docid) : null;
Review Comment:
`cacheValue` would be a better name
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
dsmiley commented on code in PR #3380:
URL: https://github.com/apache/solr/pull/3380#discussion_r2127571321
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -38,14 +40,23 @@
* @since solr 4.0
*/
public class ValueSourceAugmenter extends DocTransformer {
+
public final String name;
public final QParser qparser;
public final ValueSource valueSource;
+ private static final Object NULL_SENTINEL = new Object();
Review Comment:
statics should be defined above
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -38,14 +40,23 @@
* @since solr 4.0
*/
public class ValueSourceAugmenter extends DocTransformer {
+
public final String name;
public final QParser qparser;
public final ValueSource valueSource;
+ private static final Object NULL_SENTINEL = new Object();
+ private static final int MAX_PREFETCH_DEFAULT = 1000;
+ private final int maxPrefetchSize;
public ValueSourceAugmenter(String name, QParser qparser, ValueSource
valueSource) {
this.name = name;
this.qparser = qparser;
this.valueSource = valueSource;
+var localParams = qparser.getLocalParams();
+maxPrefetchSize =
+localParams != null
+? localParams.getInt("preFetchDocs", MAX_PREFETCH_DEFAULT)
+: MAX_PREFETCH_DEFAULT;
Review Comment:
oh I recall qparser.getParam(str) now, so you don't have to do the null
check and it'll look in the top level request. It doesn't have the integer
parse but you could use `"1000"` (as a literal; no constant needed!)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
yurkor commented on PR #3380: URL: https://github.com/apache/solr/pull/3380#issuecomment-2940917301 @dsmiley added use of localParams -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
yurkor commented on PR #3380: URL: https://github.com/apache/solr/pull/3380#issuecomment-2936715567 > I like it. @devs please have a glance at it. I'm ready to merge, but prefer the second opinion. @yurkor thank you. Would you mind to add it into solr/CHANGES.txt 9.9.0 Optimizations ? Added to CHANGES.txt -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17775: optimize ValueSourceAugmenter [solr]
mkhludnev commented on PR #3380: URL: https://github.com/apache/solr/pull/3380#issuecomment-2929962589 I like it. @devs please have a glance. I'm ready to merge, but prefer the second opinion. @yurkor thank you. Would you mind to add it into solr/CHANGES.txt 9.9.0 Optimizations ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
