Throw a parsing exception when boost is set in span_or query (#28390) #34112
Conversation
markharwood
added
the
:Search/Search
|
Pinging @elastic/es-search-aggs |
There was a problem hiding this comment.
@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); | |||
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
I think these 3 lines are not necessary, as each inner SpanQueryBuilder within their fromXContent should check for boost and fail if boost provided.
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
That is great and I think the only necessary change in SpanOrQueryBuilder. All other SpanQueryBuilders should have similar line in their fromXContent methods.
|
Thanks a lot for your review @mayya-sharipova, I'll update my PR and add tests. |
|
Hi @mayya-sharipova, clauses of compound span queries aren't aware that I'm not sure serialization should be changed, but to fix the reported issue I'd suggest to throw a 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. |
|
@cbismuth Your strategy of checking for |
|
Thank you for your review @mayya-sharipova 👍 I think I have enough information to implement a complete patch. |
This reverts commit 6349cb3
|
@cbuescher, unfortunately, still no luck. |
|
@cbismuth kk will watch out if this happens in our CI atm in general and ping infra about it |
|
Great, thank you @cbuescher 👍 |
|
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... |
|
Same issue :'( |
|
As per discussion with @cbuescher on Slack, I've restarted the That should resolve the issues with this worker... |
|
@elasticmachine test this please |
|
The build is green, thanks a lot @cbuescher and @fatmcgav 👍 @mayya-sharipova, this PR is ready for merge 😄 |
|
@cbismuth Merged - congratulations! I will later backport the changes to 6.6 as well |
|
That is really great 😄 thanks a lot @mayya-sharipova for your help 👍 |
cbismuth
deleted the
28390_disallow_setting_boosts_on_inner_span_queries
branch
|
PR for backport: #35967. PR doesn't need review, just to make sure the build passes. Once the build is passed, we can merge |
Relates to elastic/elasticsearch#28390 and elastic/elasticsearch#34112 (cherry picked from commit c4d9a44)
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.