Re: [PR] Document batch collection score contract [lucene]
jainankitk merged PR #16144: URL: https://github.com/apache/lucene/pull/16144 -- 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] Document batch collection score contract [lucene]
costin commented on code in PR #16144:
URL: https://github.com/apache/lucene/pull/16144#discussion_r3324626347
##
lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingLeafCollector.java:
##
@@ -47,7 +50,37 @@ class AssertingLeafCollector extends FilterLeafCollector {
@Override
public void setScorer(Scorable scorer) throws IOException {
-super.setScorer(AssertingScorable.wrap(scorer));
+Scorable wrappedScorer = AssertingScorable.wrap(scorer);
+if (wrappedScorer instanceof Scorer wrappedScorerAsScorer) {
+ super.setScorer(
+ new FilterScorer(wrappedScorerAsScorer) {
+@Override
+public float score() throws IOException {
+ assert batchCollectionDepth == 0
Review Comment:
A blanket guard would make valid scorables fail because the contract issue
is about iterator-backed scorer positioning. I've added a short comment
explaining 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] Document batch collection score contract [lucene]
costin commented on code in PR #16144:
URL: https://github.com/apache/lucene/pull/16144#discussion_r3324621896
##
lucene/core/src/java/org/apache/lucene/search/LeafCollector.java:
##
@@ -94,6 +94,11 @@ public interface LeafCollector {
* The default implementation calls {@link #collect(DocIdStream)} on a
{@link DocIdStream} that
* matches the given range.
*
+ * The {@link Scorable} set via {@link #setScorer} is not guaranteed to
be positioned on any
+ * document within the range. Implementations must not call {@link
Scorable#score()} or rely on
+ * scorer positioning inside this method. Use {@link #collect(int)} if
per-document scores are
+ * needed.
Review Comment:
Updated the javadoc.
--
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] Document batch collection score contract [lucene]
jainankitk commented on code in PR #16144:
URL: https://github.com/apache/lucene/pull/16144#discussion_r3323160537
##
lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingLeafCollector.java:
##
@@ -47,7 +50,37 @@ class AssertingLeafCollector extends FilterLeafCollector {
@Override
public void setScorer(Scorable scorer) throws IOException {
-super.setScorer(AssertingScorable.wrap(scorer));
+Scorable wrappedScorer = AssertingScorable.wrap(scorer);
+if (wrappedScorer instanceof Scorer wrappedScorerAsScorer) {
+ super.setScorer(
+ new FilterScorer(wrappedScorerAsScorer) {
+@Override
+public float score() throws IOException {
+ assert batchCollectionDepth == 0
Review Comment:
This assertion only fires when the wrapped scorable is a `Scorer`. Should we
consider extending it to the `AssertingScorable` branch?
##
lucene/core/src/java/org/apache/lucene/search/LeafCollector.java:
##
@@ -94,6 +94,11 @@ public interface LeafCollector {
* The default implementation calls {@link #collect(DocIdStream)} on a
{@link DocIdStream} that
* matches the given range.
*
+ * The {@link Scorable} set via {@link #setScorer} is not guaranteed to
be positioned on any
+ * document within the range. Implementations must not call {@link
Scorable#score()} or rely on
+ * scorer positioning inside this method. Use {@link #collect(int)} if
per-document scores are
+ * needed.
Review Comment:
Nit: rather than "must not call Scorable#score() or rely on scorer
positioning," we can phrase this as a contract on the scorer state — "the
scorer's position is undefined within this method; callers must not call
Scorable#score() and must not assume Scorable#docID() corresponds to any
document being collected."
--
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]
