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

refactor: Move facet extraction #3

Closed
wants to merge 1 commit into from

Conversation

nikcio
Copy link
Owner

@nikcio nikcio commented Jan 2, 2023

In my opinion it would be more consistent (and extensible) for the field type to be responsible for determining how facet data is indexed and queried. This is the case for range queries with IIndexRangeValueType already. e.g. introducing an IIndexFacetValueType interface would allow any field type to become facetable. The core field types already have the isFacetable parameter, but I do wonder what the impact / harm of a field always being facetable would be (other than index size...)? The ExtractFacets logic in the search executor could be moved into a method in the individual type classes, it would probably need to take the FacetsCollector as a parameter. The facet field type would need looking up at query time, but the existing GetFieldValueType method could be used for this and and the result could be easily type checked. This would also allow implementers to control how a custom field's facet behaviour works, where this is internal at present. (Shazwazza#311 (comment))

@Shazwazza
Copy link

Hey @nikcio was looking at your code changes and they seem good, just wondering if this was closed just because the parent is pending review or for other reasons?

@Shazwazza
Copy link

I've adjusted the code you had which I think looked pretty good and moves more of faceting to implementation details and moves the responsibility to the implementation of IIndexFacetValueType/IFacetField.

I've also changed the signature so that it allocates less and returns a collection instead of modifying an existing collection which I think is a cleaner approach (i.e. IEnumerable<KeyValuePair<string, IFacetResult>> ExtractFacets(FacetsCollector facetsCollector, SortedSetDocValuesReaderState sortedSetReaderState);)

@nikcio
Copy link
Owner Author

nikcio commented Feb 16, 2023

Hey @nikcio was looking at your code changes and they seem good, just wondering if this was closed just because the parent is pending review or for other reasons?

I just hadn't gotten any feedback on the changes and didn't want to overcomplicate the parent PR for no reason. But if you find it useful then Great!🎉

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