Skip to content

Conversation

@parkertimmins
Copy link
Contributor

When testing float range, the from and to values are generated then it is decided whether or not to include the bounds within the range. If the values are not included in the range, the min value is calculated as the next higher float value than from and to as the next lower float value. If from and to are immediately adjacent to each other, and bounds are not included, the lower end of the range will be larger than the upper.

When testing float range, the from and to values
are generated then it is decided whether or not to
include the bounds within the range. If the values
are not included in the range, the min value is
calculated as the next higher float value than from
and to as the next lower float value. If from and to
are immediately adjacent to each other, and bounds
are not included, the lower end of the range will
be larger than the upper
@parkertimmins parkertimmins requested a review from lkts May 22, 2025 17:26
@parkertimmins
Copy link
Contributor Author

Fixes #128200

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 labels May 22, 2025
@parkertimmins parkertimmins added >non-issue :StorageEngine/Mapping The storage related side of mappings and removed needs:triage Requires assignment of a team area label v9.1.0 labels May 22, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

var includeFrom = randomBoolean();
Float from = (float) randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE - Math.ulp(Float.MAX_VALUE), true);
var includeTo = randomBoolean();
Float to = (float) randomDoubleBetween(from + Math.ulp(from), Float.MAX_VALUE, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like it might be cleaner to set the lower bound for to to something like from + 2 * Math.ulp(from), but I'm not sure this is correct. If Math.ulp(to) is larger than Math.ulp(from), then it might be possible for nextUp(from) to be larger than nextDown(to) even is the lower bound used to compute to is increased. That said, I'd be a bit surprised if it's possible for Math.ulp(to) >= 2 * Math.ulp(from). But I don't know enough about floating point math to be sure, so I picked the easier way that I'm sure works.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would work but i also think we don't need this level of precision here.

Copy link
Contributor

@lkts lkts left a comment

Choose a reason for hiding this comment

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

LGTM

@parkertimmins parkertimmins merged commit a3bff18 into elastic:main May 22, 2025
18 checks passed
@parkertimmins parkertimmins deleted the parker/fix-float-range-test branch May 22, 2025 18:48
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.

3 participants