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

Enable BoostingQuery with FVH highlighter #19984

Merged
merged 3 commits into from
Aug 16, 2016
Merged

Enable BoostingQuery with FVH highlighter #19984

merged 3 commits into from
Aug 16, 2016

Conversation

chengpohi
Copy link
Contributor

@chengpohi chengpohi commented Aug 13, 2016

This PR is for enable boosting query with FVH highlighter.

#19985

@@ -56,6 +58,11 @@ public CustomFieldQuery(Query query, IndexReader reader, boolean phraseHighlight

@Override
void flatten(Query sourceQuery, IndexReader reader, Collection<Query> flatQueries, float boost) throws IOException {
while (sourceQuery instanceof BoostQuery) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When BoostQuery need to parse to other query firstly, because BoostQuery also can have BoostingQuery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CustomFieldQuery is only for FastVectorHighlighter usage.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you used while here instead of if like the rest of the cases. Do we usually have multiple layers of BoostQuery when we have a single layer? Could you push a patch with a comment about it or switch it to the normal if statement?

Copy link
Contributor Author

@chengpohi chengpohi Aug 15, 2016

Choose a reason for hiding this comment

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

Yes, After investigation, there is no directly BoostQueryBuilder api, so it should not have multiple layers BootQuery(recursive BoostQuery).

If in future there are multi layer BoostQuery, I think this should be call out. :)

@@ -75,6 +82,10 @@ void flatten(Query sourceQuery, IndexReader reader, Collection<Query> flatQuerie
} else if (sourceQuery instanceof ToParentBlockJoinQuery) {
ToParentBlockJoinQuery blockJoinQuery = (ToParentBlockJoinQuery) sourceQuery;
flatten(blockJoinQuery.getChildQuery(), reader, flatQueries, boost);
} else if (sourceQuery instanceof BoostingQuery) {
BoostingQuery boostingQuery = (BoostingQuery) sourceQuery;
flatten(boostingQuery.getContext(), reader, flatQueries, boostingQuery.getBoost());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be boost * boostingQuery.getBoost()? I think that is closer to the way that the boosts are applied.

Copy link
Contributor Author

@chengpohi chengpohi Aug 13, 2016

Choose a reason for hiding this comment

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

Oh, Yeah, I will update, Thanks

@nik9000
Copy link
Member

nik9000 commented Aug 13, 2016

I am not sure why the before doesn't enable BoostingQuery in CustomFieldQuery.

It was an oversight. A leftover during the Lucene upgrade I believe.

@nik9000 nik9000 merged commit 2adc2a1 into elastic:master Aug 16, 2016
@nik9000
Copy link
Member

nik9000 commented Aug 16, 2016

OK! All tests passed locally. Merged. Thanks agains @chengpohi !

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

Successfully merging this pull request may close these issues.

3 participants