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

Java api: add missing support for boost to GeoShapeQueryBuilder and TermsQueryBuilder #11810

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Jun 22, 2015

Both parsers support boost and parse it properly but when we create the query through java api we couldn't set the boost.

Relates to #11744

@jpountz
Copy link
Contributor

jpountz commented Jun 23, 2015

LGTM

This made me learn about BoostableQueryBuilder, could we open a follow-up issue to make this class pkg-private?

@javanna
Copy link
Member Author

javanna commented Jun 23, 2015

if we make BoostableQueryBuilder package private, custom queries cannot implement it unless they are in the same package? Not sure that's good for plugins.

@jpountz
Copy link
Contributor

jpountz commented Jun 23, 2015

My understanding is that this interface is just a way to avoid re-defining the boost method (including javadocs) on every builder since core has tens of queries. Is it more than that? If no, then plugins could just make their queries implement setBoost without inheriting this interface, or even duplicate this interface?

@javanna
Copy link
Member Author

javanna commented Jun 23, 2015

yes they could do that, but why would they? What's the advantage of making the interface package private in core?

@javanna javanna force-pushed the enhancement/query_builder_missing_boost branch from 3222e0a to 90a7b48 Compare June 23, 2015 07:26
@javanna javanna merged commit 90a7b48 into elastic:master Jun 23, 2015
@kevinkluge kevinkluge removed the review label Jun 23, 2015
@jpountz
Copy link
Contributor

jpountz commented Jun 23, 2015

In my opinion, it has several benefits:

  • if you generate javadocs, the setBoost() method would appear as defined on each query builder as opposed to being inherited from this BoostableQueryBuilder class
  • BoostableQueryBuilder would not be part of the API: imagine someone starts using "instanceof BoostableQueryBuilder" to know if a query supports boosting, then modifying/removing this interface would be considered a break, although it was only added for convenience in the beginning
  • limited coupling between the core and plugins: we could change this BoostableQueryBuilder class in whatever way we want without having any impact on plugins

I think these benefit are worth having a few duplicated lines of code in plugins.

@javanna
Copy link
Member Author

javanna commented Jun 23, 2015

thanks for the explanation @jpountz , I opened #11815 for this. Also, btw we may be removing this interface at some point too in the query-refactoring branch, as part of the consolidation of queryname and boost, we will see. This change gives us the freedom to do it indeed.

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

Successfully merging this pull request may close these issues.

None yet

3 participants