-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Conversation
With the introduction of field aliases, the toQuery() method now can also return BlendedTermQuery. Added this to the list of expected query classes.
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))); |
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.
@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?
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.
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.
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.
+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.
@jimczi thanks for your comment, I pushed an update removing the assertions altogether |
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.
LGTM
Thanks @cbuescher for fixing this! |
…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
* 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 ...
* 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) ...
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