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

Facets for SearchResults not being returned which is required to implement Dynamic Facets #914

Open
fabidi opened this issue Apr 12, 2023 · 2 comments
Assignees

Comments

@fabidi
Copy link

fabidi commented Apr 12, 2023

Hi folks! I am willing to contribute the solution. Kindly see below.

Is your feature request related to a problem? Please describe.
SearchResult.Facets are not accessible via in QueryBuilderResultsImpl.java, preventing implementation of Dynamic Facets.

  • QueryBuilderResultsImpl.facet field is left as null and never set.
  • QueryBuilderResultsImpl.getSearchResult() method has default access modifier of package-private, so unable to call it from my own code to get the facets.

Describe the solution you'd like
implement a small fix/update to QueryBuilderResultsImpl so that getSearchResult() is public and/or assign the facets field.
I can contribute it.

Describe alternatives you've considered
a) Copy QuerySearchProviderImpl, QueryBuilderResultsImpl and all the necessary non-exposed impl classes into my code base just so I can make this one change. In combination with updating properties.html and options.html, I now have a working dynamic facets implementation.
b) Using Solr indexing instead.

Additional context
See the behavior of my implementation. This is the end result I am looking to contribute, but we can take it step by step.

  1. Initial State showing 2 fields
    i) Type (w/o facets);
    ii) Category (w/ Dynamic Facets, all facet counts)
    image

  2. Non-Faceted value selected, faceted values dynamically updated
    image

  3. Faceted value selected, faceted values dynamically updated
    image

@davidjgonzalez
Copy link
Contributor

davidjgonzalez commented Apr 12, 2023

This sounds interesting! If i recall, you have to add a p.facets to the query, and then pull the facets off the QB search results object and pass them through?

Would all the search components have to get updated as well to allow this to be toggled on/off per component - or what were you thinking?

Also, do you think we'd want to put guards in place to only allow this if the index is configured to support facets (since if you run p.facets=true and you dont have the index configured, it could run slow)

@fabidi
Copy link
Author

fabidi commented Apr 18, 2023

Hi! Thanks for the quick response, apologies for the late reply.

  1. There is p.faceStrategy="computed" (default) | "oak"
  2. re: Updating all search components
    I don't think so. I think only the PropertyPredicate would need to be updated to have a boolean for facets, defaulting to false.
  3. re: guards
    I have to give more thought to that, let me explain what I understand.

a) On the one hand, the default faceStrategy="computed" does not use the oak index, rather it counts every time. It does try to get from a facetCache, which by default is set disabled (max.entries=0) We have to update the OOTB config to allow facet count caching.
My findings after my initial post: Performance is terrible as we get beyond a few thousand assets (I expected as much because bad performance was mentioned in other forums)
To answer your question: I don't know how to place a guard, I would say the developer should not enable this approach for more than a few thousand assets depending on specific performance/functionality trade-off.

Other observations
For default facetStrategy=computed, the facets will only be counted for the properties included in the search. To get all facet counts for all values of a particular field, I added a hidden predicate with operation=like, value=%. Ideally, this should be removed when a user has selected any specific values for the property, but I haven't observed the performance of query phase impacted significantly by this (given that we should only be using indexed properties!)

b) facetStragey="oak". This uses the index. I however was not able to get this to work. I followed the steps in docs to add facets=true to specific lucene property indexes, however no facets are returned. Have to investigate this path further.
But to answer your question, in this approach a guard would not be necessary because facet counting would not occur as it would not be available in the lucene properties without facets=true.

In summary, I will admit I do not have a fully fleshed out solution for all asset shares, but enabling the option for developers to call getFacets() will at least allow access to OOTB AEM behavior to explore the usage on smaller scale asset shares.

Also, we would need to add these methods
i. SearchConfig.getFacetStrategy()
ii. PagePredicate.getFacetStrategy()
to allow specifying facetStrategy in the page config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants