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

Have value and count in LabelAndValue only for TaxonomyFacets #13740

Closed
wants to merge 1 commit into from

Conversation

stefanvodita
Copy link
Contributor

#13414 in we added a count to LabelAndValue as a non-breaking solution to the problem of counts not being available from facets that also do other aggregations (#11282) despite being computed (#12966).

This time we break API and make it so only TaxonomyFacets output two values, a count and an optional additional value. All other (single value) facets work as they did before #13414.

This isn't a great solution either.

  1. Not all TaxonomyFacets have two values.
  2. What if we want more than two in the future (Compute multiple aggregations in one iteration of the match-set #12546)?
  3. To avoid breaking too many of the assumptions, comparing LabelAndValues is weird. A ValueAndCount can be equal to a Number.
  4. A user of TaxonomyFacets has to cast LabelAndValue.value to access the actual value and count.

There are other ideas in #13414. I'm not sure if I like this better than what we currently have. It's better for non-TaxonomyFacets not to use the extra memory, but it makes using TaxonomyFacets more complicated, and it makes LabelAndValue complicated.

If we go ahead with this for Lucene 10, I'd separately deprecate LabelAndValue.count in 9.12.

Copy link

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!

@github-actions github-actions bot added the Stale label Sep 22, 2024
Copy link
Member

@mikemccand mikemccand left a comment

Choose a reason for hiding this comment

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

I think I like this approach. Yes it's not perfect (for the reasons you enumerated in the op), but progress-not-perfection? Not forcing non-taxonomy facets to pay the price of the LabelAndValue overhead is appealing. I left one small refactoring comment...

Given the timing of 9.12/10.0, I'm not sure we should rush this in? Can we aim for 10.1.0? Are these @lucene.experimental APIs so we are free to break in the 10.x series?

* Is a {@link Number} while holding a {@link Number} and a count. Meant as a value for {@link
* LabelAndValue}.
*/
public static class ValueAndCount extends Number {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe pull this out into its own source file?

@github-actions github-actions bot removed the Stale label Sep 23, 2024
@stefanvodita
Copy link
Contributor Author

Given the timing of 9.12/10.0, I'm not sure we should rush this in? Can we aim for 10.1.0? Are these @lucene.experimental APIs so we are free to break in the 10.x series?

LabelAndValue is non-experimental and would lose a public field. TaxonomyFacets is also not experimental and the way its results can be used is changing.
So I don't think we can push this in a minor release. I meant this PR as a quick iterative improvement for 10.0. If we wait until 11.0, we should come up with a better solution.

Copy link

github-actions bot commented Oct 8, 2024

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!

@github-actions github-actions bot added the Stale label Oct 8, 2024
@stefanvodita
Copy link
Contributor Author

I like progress-not-perfection, but looking at this again I'm not sure it's progress. To me, it seems like a lot of complexity for a little bit of efficiency, but I could be wrong. @mikemccand, if you still like it or if anyone else likes it, I'm more than happy to drive this PR forward. Otherwise, I'll close it in a few days. Worst case we open it back up when Lucene 11 comes around and we realise we don't have anything better.

@github-actions github-actions bot removed the Stale label Oct 17, 2024
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