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

Throw a parsing exception when boost is set in span_or query (#28390) #34112

Merged
merged 48 commits into from
Nov 26, 2018
Merged

Throw a parsing exception when boost is set in span_or query (#28390) #34112

merged 48 commits into from
Nov 26, 2018

Conversation

cbismuth
Copy link
Contributor

@cbismuth cbismuth commented Sep 27, 2018

This pull request is not complete will contain guards against setting boost values in compound span queries as they are ignored in Lucene.

Before going futher I'd like to have a validation from reviewers about suggested implementation.

For more details, please see issue #28390.

@markharwood markharwood added the :Search/Search Search-related issues that do not fall into other categories label Sep 27, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@cbismuth Thanks for tacking this problem. Good first attempt! I have left some comments how we may change the code.

@@ -49,7 +50,7 @@ public SpanOrQueryBuilder(SpanQueryBuilder initialClause) {
if (initialClause == null) {
throw new IllegalArgumentException("[" + NAME + "] must include at least one clause");
}
clauses.add(initialClause);
addClause(initialClause);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not necessary. The only place where we should do the check is inside fromXContent method for all span query builders : SpanOrQueryBuilder, SpanTermQueryBuilder etc.

@@ -58,7 +59,7 @@ public SpanOrQueryBuilder(SpanQueryBuilder initialClause) {
public SpanOrQueryBuilder(StreamInput in) throws IOException {
super(in);
for (QueryBuilder clause: readQueries(in)) {
clauses.add((SpanQueryBuilder) clause);
addClause((SpanQueryBuilder) clause);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not necessary. This comes from another node that presumably already failed a span query if it contains boost.
The only place where we should do the check is inside fromXContent method for all span query builders : SpanOrQueryBuilder, SpanTermQueryBuilder etc

@@ -74,10 +75,23 @@ public SpanOrQueryBuilder addClause(SpanQueryBuilder clause) {
if (clause == null) {
throw new IllegalArgumentException("[" + NAME + "] inner clause cannot be null");
}
checkBoostValue(clause);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment here: I think this is not necessary. The only place where we should do the check is inside fromXContent method for all span query builders : SpanOrQueryBuilder, SpanTermQueryBuilder etc.

@@ -115,14 +129,16 @@ public static SpanOrQueryBuilder fromXContent(XContentParser parser) throws IOEx
if (query instanceof SpanQueryBuilder == false) {
throw new ParsingException(parser.getTokenLocation(), "spanOr [clauses] must be of type span query");
}
clauses.add((SpanQueryBuilder) query);
final SpanQueryBuilder clause = (SpanQueryBuilder) query;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these 3 lines are not necessary, as each inner SpanQueryBuilder within their fromXContent should check for boost and fail if boost provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compound span_or queries can contain span_term inner queries and setting boost in span_term queries is allowed, that's why I've added this check here, do you agree with it? See this test.

}
} else {
throw new ParsingException(parser.getTokenLocation(), "[span_or] query does not support [" + currentFieldName + "]");
}
} else {
if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
boost = parser.floatValue();
throw new ParsingException(parser.getTokenLocation(), "spanOr [clauses] can't have boost value");
Copy link
Contributor

Choose a reason for hiding this comment

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

That is great and I think the only necessary change in SpanOrQueryBuilder. All other SpanQueryBuilders should have similar line in their fromXContent methods.

@cbismuth
Copy link
Contributor Author

cbismuth commented Oct 1, 2018

Thanks a lot for your review @mayya-sharipova, I'll update my PR and add tests.

@cbismuth
Copy link
Contributor Author

cbismuth commented Oct 4, 2018

Hi @mayya-sharipova, clauses of compound span queries aren't aware that boost field isn't supported. Therefore, when converting a span_or query to JSON, each clause emits a boost field (with 1.0 as default).

I'm not sure serialization should be changed, but to fix the reported issue I'd suggest to throw a ParsingException only when boost value isn't equal to default boost value (i.e. 1.0). So that JSON serialization will keep on working as it does today and parsing will throw an exception when boost has been tuned.

I've added a commit to this PR to show you the changes I'd like to make. Could you please tell me what do you think of this? Thank you.

@mayya-sharipova
Copy link
Contributor

@cbismuth Your strategy of checking for DEFAULT_BOOST and throwing an exception only if different from DEFAULT_BOOST, makes sense to me. Thanks for noticing it!

@cbismuth
Copy link
Contributor Author

cbismuth commented Oct 4, 2018

Thank you for your review @mayya-sharipova 👍 I think I have enough information to implement a complete patch.

@cbismuth
Copy link
Contributor Author

@cbuescher, unfortunately, still no luck.

11:49:59 Caused: java.io.IOException: Remote call on JNLP4-connect connection from static.149.132.251.148.clients.your-server.de/148.251.132.149:52722 failed

@cbuescher
Copy link
Member

@cbismuth kk will watch out if this happens in our CI atm in general and ping infra about it

@cbismuth
Copy link
Contributor Author

Great, thank you @cbuescher 👍

@cbuescher
Copy link
Member

There seems to have issue with CI this morning, but they should be resolved I was told and this worker seems to look okay from a first glance. Lets try again...
@elasticmachine test this please

@cbismuth
Copy link
Contributor Author

Same issue :'(

12:45:00 Caused: java.io.IOException: Remote call on JNLP4-connect connection from static.149.132.251.148.clients.your-server.de/148.251.132.149:52722 failed

@fatmcgav
Copy link

As per discussion with @cbuescher on Slack, I've restarted the jenkins-swarm on worker-844165.

That should resolve the issues with this worker...

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbismuth
Copy link
Contributor Author

The build is green, thanks a lot @cbuescher and @fatmcgav 👍

@mayya-sharipova, this PR is ready for merge 😄

@mayya-sharipova mayya-sharipova merged commit b95a4db into elastic:master Nov 26, 2018
@mayya-sharipova
Copy link
Contributor

@cbismuth Merged - congratulations! I will later backport the changes to 6.6 as well

@cbismuth
Copy link
Contributor Author

That is really great 😄 thanks a lot @mayya-sharipova for your help 👍

@cbismuth cbismuth deleted the 28390_disallow_setting_boosts_on_inner_span_queries branch November 26, 2018 18:31
@mayya-sharipova
Copy link
Contributor

PR for backport: #35967. PR doesn't need review, just to make sure the build passes. Once the build is passed, we can merge

mayya-sharipova added a commit that referenced this pull request Dec 4, 2018
Convert parsingException to deprcation warning

Substitute for #35967, backport for #34112
codebrain added a commit to elastic/elasticsearch-net that referenced this pull request Mar 13, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Mar 18, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Mar 20, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Mar 21, 2019
russcam pushed a commit to elastic/elasticsearch-net that referenced this pull request Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >bug :Search/Search Search-related issues that do not fall into other categories v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants