Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate FacetsCollector#search utility methods #13737

Merged

Conversation

javanna
Copy link
Contributor

@javanna javanna commented Sep 6, 2024

These can now be replaced by the corresponding methods added to FacetsCollectorManager that accept a FacetsCollectorManager as last argument in place of a Collector

This is the backport #13733 , minus the removal of the static methods which get instead deprecated in the 9x branch.

Relates to #11041

These can now be replaced by the corresponding methods added to FacetsCollectorManager
that accept a FacetsCollectorManager as last argument in place of a Collector

Relates to apache#11041
@javanna javanna added this to the 9.12.0 milestone Sep 6, 2024
@javanna javanna changed the title Deprecate FacetsCollector@search utility methods Deprecate FacetsCollector#search utility methods Sep 6, 2024
Copy link
Contributor

@gsmiller gsmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to me. I caught one javadoc slip-up from the commit onto main. Left a comment around that. Thanks!

@@ -54,4 +79,167 @@ public ReducedFacetsCollector(final Collection<FacetsCollector> facetsCollectors
facetsCollector -> matchingDocs.addAll(facetsCollector.getMatchingDocs()));
}
}

/** Utility method, to search and also collect all hits into the provided {@link Collector}. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot, I think I missed this in the earlier review but looks like we need a small javadoc cleanup here (need to mention FacetsCollectorManager instead of Collector right?). Same comment applies to all the new methods. Sorry for overlooking that.

I just updated these on main with a quick commit (dc47adb). Could you cherry-pick this down into the back-port as well (and feel free to tweak the language if you think of ways to improve on it). Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, I missed that. Will backport your commit!

@javanna javanna merged commit 04d0170 into apache:branch_9x Sep 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants