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

Query DSL should disallow setting boosts on inner span queries #28390

Closed
jpountz opened this issue Jan 26, 2018 · 7 comments
Closed

Query DSL should disallow setting boosts on inner span queries #28390

jpountz opened this issue Jan 26, 2018 · 7 comments
Labels
>bug :Search/Search Search-related issues that do not fall into other categories

Comments

@jpountz
Copy link
Contributor

jpountz commented Jan 26, 2018

This is a follow-up of https://discuss.elastic.co/t/troubles-with-complex-span-query-term-boosting/116106. It is a bit confusing that we are allowing boosts on inner span queries in spite of the fact that they will be ignored.

One idea to fix it is to make SpanBoostQuery a forbidden API (I'll bring up removing it from Lucene) and replacing usage of SpanBoostQuery with BoostQuery.

@s1monw
Copy link
Contributor

s1monw commented Jan 26, 2018

++ to make the fix here first

@jpountz jpountz added the help wanted adoptme label Jan 31, 2018
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
@talevy talevy added :Search/Search Search-related issues that do not fall into other categories and removed :Search/Search Search-related issues that do not fall into other categories labels Mar 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@cbismuth
Copy link
Contributor

I'd like to work on this one.

@cbismuth
Copy link
Contributor

cURL recreation script created in this Gist.

@cbismuth
Copy link
Contributor

After having a look at implementations of SpanQueryBuilder, here are below compound span queries along with their class attributes.

  • SpanNearQueryBuilder
    • SpanNearQueryBuilder#clauses
  • SpanContainingQueryBuilder
    • SpanContainingQueryBuilder#big
    • SpanContainingQueryBuilder#little
  • SpanWithinQueryBuilder
    • SpanWithinQueryBuilder#big
    • SpanWithinQueryBuilder#little
  • SpanOrQueryBuilder
    • SpanOrQueryBuilder#clauses
  • SpanNotQueryBuilder
    • SpanNotQueryBuilder#include
    • SpanNotQueryBuilder#exclude

Here are below other span queries in which boost shouldn't be disallowed as they aren't compound.

  • SpanTermQueryBuilder (uses a single SpanTermQueryBuilder in fromXContent)
  • SpanMultiTermQueryBuilder (uses a single MultiTermQueryBuilder#multiTermQueryBuilder)
  • FieldMaskingSpanQueryBuilder (uses a single SpanQueryBuilder#queryBuilder)
  • SpanFirstQueryBuilder (uses a single SpanQueryBuilder#matchBuilder)

I'll try to raise an exception when boost field is parsed in compound span queries.

@cbismuth
Copy link
Contributor

cbismuth commented Sep 27, 2018

I've opened PR #34112.

Here is a sample output when running referenced Gist.

{
  "error" : {
    "root_cause" : [
      {
        "type" : "parsing_exception",
        "reason" : "spanOr [clauses] can't have boost value",
        "line" : 18,
        "col" : 15
      }
    ],
    "type" : "parsing_exception",
    "reason" : "spanOr [clauses] can't have boost value",
    "line" : 18,
    "col" : 15
  },
  "status" : 400
}

Notes

  • Tests aren't developed yet,
  • Exception checking can be centralized to be reused in other span query builders,
  • Backward compatibility should be discussed.

But please let me know what do you think about this implementation.

@jpountz I've seen this line of code below in SpanMultiTermQueryBuilder, is it what you think about when you're talking about a forbidden API.

@mayya-sharipova
Copy link
Contributor

Closing as this was implemented on the ES side

russcam pushed a commit to elastic/elasticsearch-net that referenced this issue Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

No branches or pull requests

7 participants