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

[CI] RangeQueryBuilderTests fails reproducibly #40937

Closed
tvernum opened this issue Apr 8, 2019 · 3 comments · Fixed by #41160
Closed

[CI] RangeQueryBuilderTests fails reproducibly #40937

tvernum opened this issue Apr 8, 2019 · 3 comments · Fixed by #41160
Assignees
Labels
:Search/Search Search-related issues that do not fall into other categories >test-failure Triaged test failures from CI

Comments

@tvernum
Copy link
Contributor

tvernum commented Apr 8, 2019

It failed on CI for 7.0, but reproduces on master
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+7.0+intake/654/console

./gradlew :server:unitTest -Dtests.seed=C6257F2747659030 -Dtests.class=org.elasticsearch.index.query.RangeQueryBuilderTests -Dtests.method="testToQuery" -Dtests.security.manager=true -Dtests.locale=sk -Dtests.timezone=Asia/Kolkata -Dcompiler.java=12 -Druntime.java=12

ERROR   1.74s | RangeQueryBuilderTests.testToQuery <<< FAILURES!
   > Throwable #1: java.lang.IllegalArgumentException: min value (101) is greater than max value (100)
@tvernum tvernum added :Search/Search Search-related issues that do not fall into other categories >test-failure Triaged test failures from CI labels Apr 8, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@cbuescher cbuescher self-assigned this Apr 8, 2019
@cbuescher
Copy link
Member

This looks like a rarely triggered case that happens with range queries when from/to being just 1 unit apart and includeLower/includeHigher are both false. Looking into this.

@cbuescher
Copy link
Member

Currently (6.7+), when we issue a search on e.g. an integer_range field like this:

GET range_index/_search
{
  "query" : {
    "range" : {
      "expected_attendees": { 
        "gt" : 10,
        "lt" : 11,
        "relation" : "intersects" 
      }
    }
  }
}

We get the following error:

"failed_shards": [
      {
        "shard": 0,
        "index": "range_index",
        "node": "1xw_KWE3QvSmXBkkWgDm-w",
        "reason": {
          "type": "query_shard_exception",
          "reason": "failed to create query: {\n  \"range\" : {\n    \"expected_attendees\" : {\n      \"from\" : 10,\n      \"to\" : 11,\n      \"include_lower\" : false,\n      \"include_upper\" : false,\n      \"relation\" : \"intersects\",\n      \"boost\" : 1.0\n    }\n  }\n}",
          "index_uuid": "UAljwH81Rzudxhl817scVg",
          "index": "range_index",
          "caused_by": {
            "type": "illegal_argument_exception",
            "reason": "min value (11) is greater than max value (10)"
          }
        }
      }
    ]

This is technically right because with gt/lt this means the range calculated internally is empty. I wonder if we should fix the test so this cannot happen and keep the error or fix throwing an error and return a MatchNoDocs query here instead. But that would probably mean also not throwing errors on queries where from cleary > to:

{
  "query" : {
    "range" : {
      "expected_attendees": { 
        "gt" : 15,
        "lt" : 10,
        "relation" : "intersects" 
      }
    }
}

Or we could only prevent the error if the lower value <= higher value but the "includeUpper/Lower" setting causes the bounds to reverse...

cbuescher pushed a commit that referenced this issue Apr 16, 2019
Currently we throw an error when a range querys minimum value exceeds the
maximum value due to the fact that they are neighbouring values and both upper
and lower value are excluded from the interval.

Since this is a condition that the user usually doesn't specify conciously (at
least in the case of float and double values its difficult to see which values
are adjacent) we should ignore those "wrong" intervals and create a
MatchNoDocsQuery in those cases.

We should still throw errors with an actionable message if the user specifies
the query interval in a way that min value > max value. This PR adds those
checks and tests for those cases.

Closes #40937
cbuescher pushed a commit that referenced this issue Apr 16, 2019
Currently we throw an error when a range querys minimum value exceeds the
maximum value due to the fact that they are neighbouring values and both upper
and lower value are excluded from the interval.

Since this is a condition that the user usually doesn't specify conciously (at
least in the case of float and double values its difficult to see which values
are adjacent) we should ignore those "wrong" intervals and create a
MatchNoDocsQuery in those cases.

We should still throw errors with an actionable message if the user specifies
the query interval in a way that min value > max value. This PR adds those
checks and tests for those cases.

Closes #40937
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
Currently we throw an error when a range querys minimum value exceeds the
maximum value due to the fact that they are neighbouring values and both upper
and lower value are excluded from the interval.

Since this is a condition that the user usually doesn't specify conciously (at
least in the case of float and double values its difficult to see which values
are adjacent) we should ignore those "wrong" intervals and create a
MatchNoDocsQuery in those cases.

We should still throw errors with an actionable message if the user specifies
the query interval in a way that min value > max value. This PR adds those
checks and tests for those cases.

Closes elastic#40937
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-failure Triaged test failures from CI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants