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

HSEARCH-809 Updating faceting API #821

Closed
wants to merge 5 commits into from

Conversation

hferentschik
Copy link
Contributor

Includes fixes for:

  • HSEARCH-1600
  • HSEARCH-809
  • HSEARCH-900
  • HSEARCH-812

@hferentschik hferentschik mentioned this pull request Apr 17, 2015
@Sanne
Copy link
Member

Sanne commented Apr 17, 2015

Awesome :)

Could I extract and apply things like HSEARCH-1600 separately?

@hferentschik
Copy link
Contributor Author

Could I extract and apply things like HSEARCH-1600 separately?

I am confused, HSEARCH-1600 is already separate. The faceting code has also already been reviewed in most parts. Can we not discuss this pull request as is.

@hferentschik hferentschik force-pushed the HSEARCH-809 branch 2 times, most recently from 6cfbf94 to 8db5c98 Compare April 18, 2015 13:11
- Introducing @facet, @facets and FacetEncodingType to configure facets
- Updating field metadata to keep faceting related information
- Updating AnnotationMetadataProvider to use additonal faceting metadata
- Making use of various DocsValue types in DocumentBuilderIndexedEntity to index facets
- Updating QueryHits to use Lucene's FacetCollector
- Updating Faceting DSL
- Updating faceting tests to make use of @facet
- Updating features.xml to export correct packages from faceting module
- Removing obsolete classes (FacetCollector and FacetCounter)
- Adding verification that indexed field configured for faceting is not analyzed
- Updating documentation to relfect changes in faceting API
…o dynamic faceting via Lucene's API (see HSEARCH-809)
@Sanne
Copy link
Member

Sanne commented Apr 18, 2015

I'm not understanding the Jenkins error. Documentation builds fine here..
I'll apply the commit for HSEARCH-1600 already just to get rid of the distraction, then I'll test the build of the manual again.

@hferentschik
Copy link
Contributor Author

You realize I pushed an update already? There was a problem with the docbook build (duplicate id) which I resolved. Did not show via asciidoctor. Just make sure to get the latest branch and take it from there.

@Sanne
Copy link
Member

Sanne commented Apr 19, 2015

right, thanks. I got confused as I had checked out the latest - which worked fine - just after you pushed apparently, as Jenkins finished the build marking it "green" again ten minutes later :)

@Sanne
Copy link
Member

Sanne commented Apr 19, 2015

I'd be very eager to merge this, not only because users are asking for it but also as I'd hope to use it as a trampoline to get to Lucene 5.

The big question is IMO about the backwards compatibility change in a minor release - by our own rules we can't do that, although I guess it's debatable if requiring the new annotations is indeed an API change.

I'd really like to say it's not and apply it, but I kinda have to admit it's because I want to, and not necessarily a fair judgement. Let's talk about it on the mailing list?

A safer approach could be to start a branch 6 already.. we could make good use of that to include Lucene 5 patches and work in progress about ORM 5 as well. But it will be a long wait to actually release a 6 since we want many more breaking changes in there. Let's chat/email about it with everyone next week?

@hferentschik
Copy link
Contributor Author

IMO we can merge and make this a feature of 5.2. As you say, a lot of people are waiting for this, in particular since it also solved the *ToMany faceting issue we always had with the collector approach. And even though one needs to add new annotations, the API itself is pretty much unchanged. Waiting for Search 6 seems to long of a wait for this feature.

+1 for merging and releasing as part of 5.2

@hferentschik
Copy link
Contributor Author

What the plan on this now. IMO we should merge asap

@Sanne
Copy link
Member

Sanne commented May 5, 2015

I'll merge this later today, need to apply some changes before this.

@hferentschik
Copy link
Contributor Author

I'll merge this later today, need to apply some changes before this.

Ok

@Sanne
Copy link
Member

Sanne commented May 5, 2015

merged!

@Sanne Sanne closed this May 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants