Re: [PR] SOLR-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3067703993 Yep I'm confident in it. It's all merged. -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley merged PR #1244: URL: https://github.com/apache/solr/pull/1244 -- 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-5707: Lucene Expressions in Solr [solr]
HoustonPutman commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3066623987 Actually I’m having issues with the smoketester erroring in the tests, so if you can get this in by Monday and are sure it wont break the tests, go for it. It seems like a new feature, so im not too worried about it breaking anything other than the 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: [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-5707: Lucene Expressions in Solr [solr]
HoustonPutman commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3064545995 @dsmiley I just started producing the 9.9.0 release, so let's leave this for 9.10. We don't have to wait months for 9.10 either, we can do it in a month or two and get this out after baking for a little bit. -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3064513883 DocList / DocIterator / DocIterationInfo != a Lucene Scorable (which is an abstract class, BTW; limits creative class hierarchy possibilities). Furthermore the former is an iterator but the latter returns the current state. These things aren't compatible. @HoustonPutman I don't want to push too last minute for this in 9.9 unless you are comfortable with this as the RM. I think this is a great feature that has waited over 11 years too long. -- 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-5707: Lucene Expressions in Solr [solr]
AndreyBozhko commented on code in PR #1244:
URL: https://github.com/apache/solr/pull/1244#discussion_r2202025727
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -68,20 +70,44 @@ public void setContext(ResultContext context) {
fcontext = ValueSource.newContext(searcher);
this.valueSource.createWeight(fcontext, searcher);
final var docList = context.getDocList();
- if (docList == null) {
+ final int prefetchSize = docList == null ? 0 : Math.min(docList.size(),
maxPrefetchSize);
+ if (prefetchSize == 0) {
return;
}
- final int prefetchSize = Math.min(docList.size(), maxPrefetchSize);
+ // Check if scores are wanted and initialize the Scorable if so
+ final MutableScorable scorable; // stored in fcontext (when not null)
+ final IntFloatHashMap docToScoreMap;
+ if (context.wantsScores()) { // TODO switch to ValueSource.needsScores
once it exists
+docToScoreMap = new IntFloatHashMap(prefetchSize);
Review Comment:
It looked to me that the mapping between docIds and scores is already
established in the DocList (so why do the mapping again?), and only the sorting
piece is missing. That's why I was thinking of a way to handle just the sorting.
But I agree - if this is the only place, then I'm OK with the logic as long
as it works.
--
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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3059200258 > making sure functions can access scores in distributed queries, What would go wrong if this functionality were used in such? An aside: I have a wish for our tests to be able to run in both modes easily. I've used that strategy at work. The test here is based on our oldest test infrastructure, and that which wouldn't easily support that. I'd like to see further use of the new `SolrClientTestRule` such as using SolrCloud; there's a JIRA for that. > exposing score to functions that act as post-filters. I saw that one-liner you added for `frange` specifically (which I wish wasn't a PostFilter, but I digress). Is there a more general adaptation? Do you have concerns with the merge-ability of this PR as it stands for 9.9? -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on code in PR #1244:
URL: https://github.com/apache/solr/pull/1244#discussion_r2198765440
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -68,20 +70,44 @@ public void setContext(ResultContext context) {
fcontext = ValueSource.newContext(searcher);
this.valueSource.createWeight(fcontext, searcher);
final var docList = context.getDocList();
- if (docList == null) {
+ final int prefetchSize = docList == null ? 0 : Math.min(docList.size(),
maxPrefetchSize);
+ if (prefetchSize == 0) {
return;
}
- final int prefetchSize = Math.min(docList.size(), maxPrefetchSize);
+ // Check if scores are wanted and initialize the Scorable if so
+ final MutableScorable scorable; // stored in fcontext (when not null)
+ final IntFloatHashMap docToScoreMap;
+ if (context.wantsScores()) { // TODO switch to ValueSource.needsScores
once it exists
+docToScoreMap = new IntFloatHashMap(prefetchSize);
Review Comment:
The sorting & mapping has to happen somewhere. It's inelegant. Is your
point to add a convenience API/method? If we do this in multiple places, then
that'd make sense, but not otherwise.
--
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-5707: Lucene Expressions in Solr [solr]
AndreyBozhko commented on code in PR #1244:
URL: https://github.com/apache/solr/pull/1244#discussion_r2198440430
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -68,20 +70,44 @@ public void setContext(ResultContext context) {
fcontext = ValueSource.newContext(searcher);
this.valueSource.createWeight(fcontext, searcher);
final var docList = context.getDocList();
- if (docList == null) {
+ final int prefetchSize = docList == null ? 0 : Math.min(docList.size(),
maxPrefetchSize);
+ if (prefetchSize == 0) {
return;
}
- final int prefetchSize = Math.min(docList.size(), maxPrefetchSize);
+ // Check if scores are wanted and initialize the Scorable if so
+ final MutableScorable scorable; // stored in fcontext (when not null)
+ final IntFloatHashMap docToScoreMap;
+ if (context.wantsScores()) { // TODO switch to ValueSource.needsScores
once it exists
+docToScoreMap = new IntFloatHashMap(prefetchSize);
Review Comment:
This makes me wish for something like
```java
DocList sortedDocList = docList
.subset(docList.offset(), prefetchSize)
.sortedByDocId();
```
and then wrap the sortedDocList.iterator() as a Scorable. This could remove
the need for a custom mapping between docids and scores in ValueSourceAugmenter.
--
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-5707: Lucene Expressions in Solr [solr]
bruno-roustant commented on code in PR #1244: URL: https://github.com/apache/solr/pull/1244#discussion_r2195575657 ## solr/solr-ref-guide/modules/query-guide/pages/expression-value-source-parser.adoc: ## @@ -0,0 +1,56 @@ += Expression Value Source Parser +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +The `ExpressionValueSourceParser` allows you to implement a custom valueSource merely by adding a concise JavaScript expression to your `solrconfig.xml`. +The expression is precompiled, and offers competitive performance to those written in Java. +The syntax is a limited subset of JavaScript that is purely numerical oriented, and only certain built-in fuctions can be called. Review Comment: Typo "fuctions" -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3051138867 @AndreyBozhko you might be a good reviewer 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3047156236 I cherry-picked the Lucene 9.12.2 upgrade (thanks @HoustonPutman ), removed the `Ignore` annotation from the 2 tests, fixed a minor test bug that one Ignored test was hiding, and confirmed that this Lucene upgrade addressed the lingering concern :-) I didn't push the Lucene upgrade commit so as to not confuse this PR, and thus those 2 tests will fail. @houston not sure if you have concerns on this getting into 9.9. -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-3044907028 @hossman as the original author, maybe you'd like to review 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: [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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-2875034096 The ability to reference scores in a custom ValueSource based on DoubleValuesSource won't work with sorting. I filed this PR https://github.com/apache/lucene/pull/14654 to Lucene to trivially fix that issue. But since Lucene 9.x is EOL; I don't know when we might expect that if ever in Solr 9. -- 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]
[PR] SOLR-5707: Lucene Expressions in Solr [solr]
risdenk opened a new pull request, #1244: URL: https://github.com/apache/solr/pull/1244 https://issues.apache.org/jira/browse/SOLR-5707 This is me taking Hoss's VSP patch from https://issues.apache.org/jira/browse/SOLR-5707 and trying to look at it again. - [x] `./gradlew check -x test -Pvalidation.errorprone=true -Pvalidation.sourcePatterns.failOnError=false` passes :) - [ ] nocommit are still in there - [ ] do we NEED to deal w/ score? can we do score different now that its been years later - [ ] does `./gradlew -p solr/core test --tests ExpressionValueSourceParserTest` pass? - [ ] do all tests pass? - [ ] how does this perform - this is my real goal here compared to other say boostfunctions -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-2855399596 Updating the PR to main was easy. Note that there is another PR #3340 that will overlap with this one which makes it possible for a DocTransformer to access the score. The failing tests relate to sorting by an expression that makes use of the score. If we mark those tests with an `Ignore` and add a follow-up to improve that matter, then I think we have something mergeable. Progress not perfection; right? Addressing the score issue is complicated by the fact that Lucene's `ValueSource` API is legacy, has no concept of `needsScores`, but the replacement `DoubleValuesSource` _does_ have this concept. Solr is still ValueSource based. I could see doing a switch for Solr 10 but that's for another issue and dev list discussion. -- 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-5707: Lucene Expressions in Solr [solr]
risdenk commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-2845738147 Go for it @dsmiley I ended up not needing it but it did work when I was trying it -- 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-5707: Lucene Expressions in Solr [solr]
dsmiley commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-2845732457 I commented in JIRA about my interest in furthering this. I'm sure there are some nice-to-have's (score function access, documentation), but simply working code & tested is enough to merge. I can brush the dust off get this PR mergeable; what do you say @risdenk ? This functionality plugs in nicely, showcasing Solr's great abstractions without needing to hack on any Solr plumbing. I would later expand the usage to support access to text fields via a DocTransformer use case but I think another PR can do that on top of the fine work already done 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-5707: Lucene Expressions in Solr [solr]
risdenk commented on PR #1244: URL: https://github.com/apache/solr/pull/1244#issuecomment-2448124184 Not looking at this anymore -- 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-5707: Lucene Expressions in Solr [solr]
risdenk closed pull request #1244: SOLR-5707: Lucene Expressions in Solr URL: https://github.com/apache/solr/pull/1244 -- 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]
