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

Use lt instead of lte for safer upper bound in range filter. #7129

Merged
merged 1 commit into from
May 11, 2016

Conversation

ycombinator
Copy link
Contributor

Using lte with +interval -1ms assumes that 1ms is the smallest delta possible. This may be true today but may not stay true in the future. It is safer to use lt with +interval instead.

Discovered by @rashidkpc while reviewing #7126.

Using lte with +interval -1ms assumes that 1ms is the smallest delta possible.
This may be true today but may not stay true in the future. It is safer to
use lt with +interval instead.
@rashidkpc
Copy link
Contributor

@spalger looks like you added the lte w/ .subtract(1, 'ms') any reason not to use lt instead? Something obvious we missed?

@ycombinator
Copy link
Contributor Author

Summarizing the chat in IRC about this amongst @spalger, @rashidkpc and me:

The lte was being used so the filter bar displayed the filter like so:

screen shot 2016-05-04 at 9 21 53 am

Using lt instead of lte makes the filter bar display the filter like so:

screen shot 2016-05-04 at 9 22 33 am

Note the difference in upper bounds shown in the filter bar, specifically the second and milliseconds values. The upper bound in the first image is inclusive while the one in the second image is exclusive.

@spalger
Copy link
Contributor

spalger commented May 11, 2016

LGTM

@spalger spalger merged commit 0051da4 into elastic:master May 11, 2016
@ycombinator ycombinator deleted the safer-range-bound branch May 31, 2016 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants