-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Fix range query edge cases #41160
Fix range query edge cases #41160
Conversation
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.
Pinging @elastic/es-search |
@not-napoleon you showed some interest in this case, would you mind taking a look? |
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.
Left comments that need addressing. Also, this is going to conflict with a PR I'm working on, but you should merge first and I'll fix the conflict in my branch. This is fixing an actual bug, and I still have test cases to write.
// wrong argument order, this is an error the user should fix | ||
throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); | ||
} | ||
Double correctedFrom = (Double) from + (includeFrom ? 0 : 1); |
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 increasing by 1 here is wrong; that's too big a jump for doubles. We should use RangeType.DOUBLE.nextUp()
here, I think. E.g., if a user specifies the exclusive range (0, 1)
, we would expect it to intersect the range (.3, .7)
, but this logic will return MatchNoDocsQuery
.
In fact, I wonder if we should just have one createQuery
function that's written in terms ofnextUp()
, possibly accepting a comparator too, if necessary.
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.
Agreed, I had this in an earlier version of this PR, must have slipped, will update.
// wrong argument order, this is an error the user should fix | ||
throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); | ||
} | ||
Float correctedFrom = (Float) from + (includeFrom ? 0 : 1); |
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.
See below comment for doubles ranges; floats should also use nextUp()
instead of just increasing by 1.
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 I'll try creating only one createQuery function...
@not-napoleon thanks for the feedback, I changed to nextUp/Down and created a single createQuery() method for the numeric types. I left IP out since its a special kind of snowflake and probably easier to handle on its own. |
LGTM. If it was me, I'd put a comment on the static |
Since there are still test failures that require another re-run I might just do that ;-) |
@elasticmachine run elasticsearch-ci/packaging-sample |
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
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
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