Re: [PR] FilterLeafCollector should delegate competitiveIterator() [lucene]
github-actions[bot] commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4949360331 This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! -- 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
mrkm4ntr commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4805809706 @javanna HDYT? -- 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
github-actions[bot] commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4794905773 This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! -- 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
mrkm4ntr commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4665971116 That's an interesting direction for a future major version. Making these default implementations non-default would indeed eliminate the ambiguity around what Filter* classes should delegate. That said, I think it's out of scope for this PR. Under the current design, FilterLeafCollector is a delegator that already delegates setScorer(), collect(), and finish() — adding competitiveIterator() is consistent with that role. I'd prefer to keep this fix in FilterLeafCollector for now, and the broader interface design discussion can be tracked separately. -- 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
msfroh commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4615508096 For these cases where we've added a default implementation in an interface / abstract class, I get the impression that we're often doing it to avoid breaking consumers (unless we make the implementation `final` in an abstract class, in which case we're being prescriptive about the implementation). I wonder if there's some value going forward to suggest moving these "convenient"/non-breaking default implementations out of the interfaces on the next major version, forcing everyone to implement them? We can still define a helpful abstract base class with default implementations for use in concrete subclasses (like `BaseDirectory`), but the `Filter*` classes can then delegate the given methods (borrowing that idea from [this issue](https://github.com/apache/lucene/issues/14985#issuecomment-3115980036)). As we've discussed before, finding a perfect solution to the `Filter*` problem is really hard. -- 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
mrkm4ntr commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4612510648 I understand the concern given the experience with #15469, but I think competitiveIterator() is different from the nextDoc()/intoBitSet() case that caused the revert. That issue was caused by inter-method dependencies in DocIdSetIterator's default implementations — delegating both methods broke the invariant between them. competitiveIterator() has no such dependency on other methods, so delegating it in FilterLeafCollector should be safe. FilterLeafCollector exists so that subclasses can customize only collect() and setScorer() without worrying about the rest. If competitiveIterator() is not delegated by default, every subclass that only wants to customize those two methods has to remember to also override competitiveIterator() — which defeats the purpose of having a delegator base class. -- 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
javanna commented on PR #16133: URL: https://github.com/apache/lucene/pull/16133#issuecomment-4611011414 I was reminded of previous discussions about the pattern to follow in Filter* classes, as mentioned in #15469 and other related conversations. Given `competitiveIterator` is a default method and not abstract, perhaps we should leave this as-is, and instead correct the subclasses that need to override 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] FilterLeafCollector should delegate competitiveIterator() [lucene]
mrkm4ntr commented on code in PR #16133:
URL: https://github.com/apache/lucene/pull/16133#discussion_r3339407328
##
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestProfilerCollector.java:
##
@@ -86,6 +94,35 @@ protected String deriveCollectorName(Collector c) {
assertEquals("TestCollector(TotalHitCountCollector)", collector.getName());
}
+ public void testCompetitiveIteratorDelegation() throws IOException {
Review Comment:
Testing FilterLeafCollector directly for competitiveIterator() delegation
would be tautological — it would just assert that the method calls through to
in. The same applies to setScorer, collect, and finish, none of which have
delegation tests either. There's no reason to treat competitiveIterator
differently. The sandbox test (testCompetitiveIteratorDelegation) verifies the
delegation indirectly through a real use case with ProfilerCollectorWrapper.
##
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestProfilerCollector.java:
##
@@ -86,6 +94,35 @@ protected String deriveCollectorName(Collector c) {
assertEquals("TestCollector(TotalHitCountCollector)", collector.getName());
}
+ public void testCompetitiveIteratorDelegation() throws IOException {
+Directory dir = newDirectory();
+RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+final int numDocs = TestUtil.nextInt(random(), 10, 100);
+for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
+ doc.add(new LongPoint("score", i));
+ doc.add(new NumericDocValuesField("score", i));
+ w.addDocument(doc);
+}
+w.forceMerge(1);
+DirectoryReader reader = w.getReader();
+w.close();
+
+SortField sortField = new SortField("score", SortField.Type.LONG);
+TopFieldCollectorManager topFieldCollectorManager =
+new TopFieldCollectorManager(new Sort(sortField), 1,
Integer.MAX_VALUE);
+
+ProfilerCollectorWrapper wrapper =
+new ProfilerCollectorWrapper(topFieldCollectorManager.newCollector());
+LeafCollector leafCollector =
wrapper.getLeafCollector(reader.leaves().get(0));
+
+DocIdSetIterator competitiveIterator = leafCollector.competitiveIterator();
+assertNotNull(competitiveIterator);
Review Comment:
The correctness of the competitive iterator itself (e.g., that it properly
skips non-competitive documents) is already tested in TestNumericComparator and
related tests. The responsibility of this fix is ensuring that
FilterLeafCollector delegates competitiveIterator() rather than silently
returning null. Verifying non-null is sufficient to confirm delegation is
working, and going further would be testing the comparator's behavior rather
than the fix itself.
--
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] FilterLeafCollector should delegate competitiveIterator() [lucene]
javanna commented on code in PR #16133:
URL: https://github.com/apache/lucene/pull/16133#discussion_r3322804990
##
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestProfilerCollector.java:
##
@@ -86,6 +94,35 @@ protected String deriveCollectorName(Collector c) {
assertEquals("TestCollector(TotalHitCountCollector)", collector.getName());
}
+ public void testCompetitiveIteratorDelegation() throws IOException {
Review Comment:
This is a good test that's specific to the profiler collector, thanks for
adding it
The fix is general, hence it may be good to add a test that subclasses
`FilterLeafCollector` directly and verifies that delegating is a functionality
provided directly by `FilterLeafCollector`. Otherwise the fix could be specific
to the profilter collector wrapper.
##
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestProfilerCollector.java:
##
@@ -86,6 +94,35 @@ protected String deriveCollectorName(Collector c) {
assertEquals("TestCollector(TotalHitCountCollector)", collector.getName());
}
+ public void testCompetitiveIteratorDelegation() throws IOException {
+Directory dir = newDirectory();
+RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+final int numDocs = TestUtil.nextInt(random(), 10, 100);
+for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
+ doc.add(new LongPoint("score", i));
+ doc.add(new NumericDocValuesField("score", i));
+ w.addDocument(doc);
+}
+w.forceMerge(1);
+DirectoryReader reader = w.getReader();
+w.close();
+
+SortField sortField = new SortField("score", SortField.Type.LONG);
+TopFieldCollectorManager topFieldCollectorManager =
+new TopFieldCollectorManager(new Sort(sortField), 1,
Integer.MAX_VALUE);
+
+ProfilerCollectorWrapper wrapper =
+new ProfilerCollectorWrapper(topFieldCollectorManager.newCollector());
+LeafCollector leafCollector =
wrapper.getLeafCollector(reader.leaves().get(0));
+
+DocIdSetIterator competitiveIterator = leafCollector.competitiveIterator();
+assertNotNull(competitiveIterator);
Review Comment:
Could we check that the competitive iterator returned is meaningful, and
provides the expected functionality, besides it being non null?
--
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] FilterLeafCollector should delegate competitiveIterator() [lucene]
mrkm4ntr commented on code in PR #16133:
URL: https://github.com/apache/lucene/pull/16133#discussion_r3318756896
##
lucene/core/src/java/org/apache/lucene/search/FilterLeafCollector.java:
##
@@ -42,6 +42,11 @@ public void collect(int doc) throws IOException {
in.collect(doc);
}
+ @Override
+ public DocIdSetIterator competitiveIterator() throws IOException {
+return in.competitiveIterator();
Review Comment:
Thanks. Added a 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: [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] FilterLeafCollector should delegate competitiveIterator() [lucene]
javanna commented on code in PR #16133:
URL: https://github.com/apache/lucene/pull/16133#discussion_r3317094018
##
lucene/core/src/java/org/apache/lucene/search/FilterLeafCollector.java:
##
@@ -42,6 +42,11 @@ public void collect(int doc) throws IOException {
in.collect(doc);
}
+ @Override
+ public DocIdSetIterator competitiveIterator() throws IOException {
+return in.competitiveIterator();
Review Comment:
Could you add a test please?
--
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] FilterLeafCollector should delegate competitiveIterator() [lucene]
mrkm4ntr opened a new pull request, #16133: URL: https://github.com/apache/lucene/pull/16133 ### Description `FilterLeafCollector` delegates `setScorer()`, `collect()`, and `finish()` to the wrapped `LeafCollector`, but does not delegate `competitiveIterator()`. This causes the default implementation from the `LeafCollector` interface (`return null`) to be used, which disables dynamic pruning for all `FilterLeafCollector` subclasses. One notable case is `ProfilerCollectorWrapper` in the sandbox module. When profiling is enabled, sorted queries on numeric/date fields lose the competitive iterator provided by `TopFieldCollector`, falling back to a full scan of all matching documents. -- 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]
