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

[Tests] Adapt QueryStringQueryBuilderTests expectations #32236

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented Jul 20, 2018

With the introduction of field aliases, the toQuery() method now can also return
BlendedTermQuery. Added this to the list of expected query classes.

Closes #32234

With the introduction of field aliases, the toQuery() method now can also return
BlendedTermQuery. Added this to the list of expected query classes.
@cbuescher cbuescher added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories labels Jul 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

.or(instanceOf(BlendedTermQuery.class)).or(instanceOf(PhraseQuery.class))
.or(instanceOf(BoostQuery.class)).or(instanceOf(MultiPhrasePrefixQuery.class))
.or(instanceOf(PrefixQuery.class)).or(instanceOf(SpanQuery.class))
.or(instanceOf(MatchNoDocsQuery.class)));
Copy link
Member Author

Choose a reason for hiding this comment

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

@jimczi @jtibshirani the query string query that gets translated to a Lucene BlendedTermQuery in the previously failing test is:

{
  "query_string" : {
    "query" : "sMPjDLuo ",
    "fields" : [
      "mapped_string^0.4345442",
      "mapped_string_alias^1.0"
    ],
    "type" : "cross_fields",
    "tie_breaker" : 0.5032719,
    "default_operator" : "or",
    "max_determinized_states" : 10000,
    "allow_leading_wildcard" : true,
    "enable_position_increments" : true,
    "fuzziness" : "1",
    "fuzzy_prefix_length" : 0,
    "fuzzy_max_expansions" : 50,
    "phrase_slop" : 3,
    "rewrite" : "scoring_boolean",
    "minimum_should_match" : "-25%",
    "quote_field_suffix" : "Qcu",
    "escape" : false,
    "auto_generate_synonyms_phrase_query" : true,
    "fuzzy_transpositions" : true,
    "boost" : 1.0,
    "_name" : "paAge15"
  }
}

It includes the newly introduced alias fields, so I assume the behaviour is correct, but can you re-check that adding this to the expected list of query classes is okay?

Copy link
Member Author

Choose a reason for hiding this comment

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

On a second note, I also wouldn't be against removing these assertions altogether. In my opinion this makes the test more brittle and doesn't achieve much in terms of assertions, since the list of accepted classes is quite long already. Let me know if you also would approve if we remove this altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to remove these assertions, the query_string query can build all sort of queries so I don't think we should track the full list here. This will also allow more flexibility in the random query generator.

@cbuescher
Copy link
Member Author

@jimczi thanks for your comment, I pushed an update removing the assertions altogether

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM

@cbuescher cbuescher merged commit 54d896c into elastic:master Jul 20, 2018
@jtibshirani
Copy link
Contributor

Thanks @cbuescher for fixing this!

cbuescher pushed a commit that referenced this pull request Jul 20, 2018
…32236)

Currently we check that the queries that QueryStringQueryBuilder#toQuery returns
is one out of a list of many Lucene query classes. This list has extended a lot over time,
since QueryStringQueryBuilder can build all sort of queries. This makes the test hard to
maintain. The recent addition of alias fields which build a BlendedTermQuery show how
easy this test breaks. Also the current assertions doesn't add a lot in terms of catching
errors. This is why we decided to remove this check.

Closes #32234
martijnvg added a commit that referenced this pull request Jul 21, 2018
* es/6.x: (24 commits)
  Fix broken backport
  Switch full-cluster-restart to new style Requests (#32140)
  Fix multi level nested sort (#32204)
  MINOR: Remove unused `IndexDynamicSettings` (#32237) (#32248)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Switch rolling restart to new style Requests (#32147)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Enable testing in FIPS140 JVM (#31666) (#32231)
  Remove indices stats timeout from monitoring docs
  TESTS: Check for Netty resource leaks (#31861) (#32225)
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Backport SSL context names (#30953) to 6.x (#32223)
  Require Gradle 4.9  as minimum version (#32200)
  Detect old trial licenses and mimic behaviour (#32209)
  Painless: Simplify Naming in Lookup Package (#32177)
  add support for write index resolution when creating/updating documents (#31520)
  A replica can be promoted and started in one cluster state update (#32042)
  Rest test - allow for snapshots to take 0 milliseconds
  ...
martijnvg added a commit that referenced this pull request Jul 21, 2018
* es/master: (23 commits)
  Switch full-cluster-restart to new style Requests (#32140)
  [DOCS] Clarified that you must remove X-Pack plugin when upgrading from pre-6.3. (#32016)
  Remove BouncyCastle dependency from runtime (#32193)
  INGEST: Extend KV Processor (#31789) (#32232)
  INGEST: Make a few Processors callable by Painless (#32170)
  Add region ISO code to GeoIP Ingest plugin (#31669)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Make sure that field aliases count towards the total fields limit. (#32222)
  Switch rolling restart to new style Requests (#32147)
  muting failing test for internal auto date histogram to avoid failure before fix is merged
  MINOR: Remove unused `IndexDynamicSettings` (#32237)
  Fix multi level nested sort (#32204)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Remove indices stats timeout from monitoring docs
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Remove aliases resolution limitations when security is enabled (#31952)
  Ensure that field aliases cannot be used in multi-fields. (#32219)
  TESTS: Check for Netty resource leaks (#31861)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants