-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Changes from 2 commits
6c12c4a
265acde
268b78e
6effd0a
6349cb3
e8b61bd
04f66ce
8029eb2
0f95a83
e94a942
7c348bf
a20d761
e39bf1d
bee3ef6
c48777c
9426ffe
af54fc8
87da533
95ff5ed
7d25d14
1f08196
daea8b1
70cffbe
9e433b3
006b37d
8f8e035
e8ec5c1
a5dd5b8
49ce791
0ac11ec
a88866e
9c751b1
8a75747
8e9c70d
f2861cd
df03f38
11f6060
043020b
c96aaa9
3b667bf
259c446
e486466
78e8c21
16127d9
1fb94ee
94b5bcb
dc1e913
309e00b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.xcontent.XContentBuilder; | ||
import org.elasticsearch.common.xcontent.XContentLocation; | ||
import org.elasticsearch.common.xcontent.XContentParser; | ||
|
||
import java.io.IOException; | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe 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. |
||
clauses.add(clause); | ||
return this; | ||
} | ||
|
||
private static void checkBoostValue(SpanQueryBuilder clause) { | ||
if (clause.boost() != AbstractQueryBuilder.DEFAULT_BOOST) { | ||
throw new IllegalArgumentException("spanOr [clauses] can't have boost value"); | ||
} | ||
} | ||
|
||
private static void checkBoostValue(XContentLocation location, SpanQueryBuilder clause) { | ||
if (clause.boost() != AbstractQueryBuilder.DEFAULT_BOOST) { | ||
throw new ParsingException(location, "spanOr [clauses] can't have boost value"); | ||
} | ||
} | ||
|
||
/** | ||
* @return the {@link SpanQueryBuilder} clauses that were set for this query | ||
*/ | ||
|
@@ -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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compound |
||
checkBoostValue(parser.getTokenLocation(), clause); | ||
clauses.add(clause); | ||
} | ||
} 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is great and I think the only necessary change in |
||
} else if (AbstractQueryBuilder.NAME_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { | ||
queryName = parser.text(); | ||
} else { | ||
|
There was a problem hiding this comment.
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.