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

Backport to 9x: Initialize facet counting data structures lazily #12408 #13300

Merged
merged 2 commits into from
May 3, 2024

Conversation

stefanvodita
Copy link
Contributor

@stefanvodita stefanvodita commented Apr 13, 2024

We can bring this optimisation from #12408 to 9x. It would make it easier to backport #12966 afterwards. I think the intention was to backport at first, since the CHANGES entry is under 9.8 in the commit.

This change covers:
* Taxonomy faceting
  * FastTaxonomyFacetCounts
  * TaxonomyFacetIntAssociations
  * TaxonomyFacetFloatAssociations
* SSDV faceting
  * SortedSetDocValuesFacetCounts
  * ConcurrentSortedSetDocValuesFacetCounts
  * StringValueFacetCounts
* Range faceting:
  * LongRangeFacetCounts
  * DoubleRangeFacetCounts
* Long faceting:
  * LongValueFacetCounts

Left for a future iteration:
* RangeOnRange faceting
* FacetSet faceting
@stefanvodita
Copy link
Contributor Author

@gsmiller - I'd like your opinion as the author of the original commit. Am I missing some reason why we shouldn't backport?

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 dev@lucene.apache.org list. Thank you for your contribution!

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

I went through this change once more to make sure it's safe to push. I would still like a second opinion, just in case I'm missing something, otherwise I'll merge this in a few days.

Copy link
Contributor

@msokolov msokolov left a comment

Choose a reason for hiding this comment

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

it seems reasonable, if a bit messy (so many conditionals!) ... but this is what was committed to main branch, right? Did you have to make many changes to pull it in to 9x? Anyway LGTM

@stefanvodita
Copy link
Contributor Author

Thank you for the review Mike! I did have to change a couple things:

  1. Added a new constructor to TaxonomyFacets, which takes a FacetsCollector, since I couldn't change the existing one.
  2. Updated TaxonomyFacetCounts too, which no longer exists on main.

@github-actions github-actions bot removed the Stale label May 2, 2024
@stefanvodita stefanvodita merged commit 26ec4c6 into apache:branch_9x May 3, 2024
2 checks passed
@stefanvodita stefanvodita added this to the 9.11.0 milestone May 3, 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.

None yet

3 participants