Scale doubles to floats when necessary to match the field (#78344)#8
Scale doubles to floats when necessary to match the field (#78344)#8MitchLewis930 wants to merge 1 commit intopr_018_beforefrom
Conversation
) This fixes a bug where the range aggregation always treats the range end points as doubles, even if the field value doesn't have enough resolution to fill a double. This was creating issues where the range would have a "more precise" approximation of an unrepresentable number than the field, causing the value to fall in the wrong bucket. Note 1: This does not resolve the case where we have a long value that is not precisely representable as a double. Since the wire format sends the range bounds as doubles, by the time we get to where this fix is operating, we've already lost the precision to act on a big long. Fixing that problem will require a wire format change, and I'm not convinced it's worth it right now. Note 2: This is probably still broken for ScaledFloats, since they don't implement NumberFieldType. Resolves elastic#77033
There was a problem hiding this comment.
Pull request overview
This PR fixes a precision issue in range aggregations where floating-point values (float, double, half_float) stored with limited precision could incorrectly fall outside their expected ranges due to representation differences between double boundaries and stored values.
Changes:
- Introduced precision reduction logic to adjust range boundaries to match the stored field precision
- Added comprehensive test coverage for float, double, and half_float range endpoint behavior
- Updated integration tests to reflect corrected range aggregation behavior
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| NumberFieldMapper.java | Added reduceToStoredPrecision method to convert double values to field-specific precision |
| ValuesSourceConfig.java | Added reduceToStoredPrecisionFunction to expose precision reduction for field types |
| RangeAggregationBuilder.java | Applied precision reduction to range boundaries before processing |
| RangeAggregatorTests.java | Added unit tests for float, double, and half_float exclusive endpoint behavior |
| 40_range.yml | Added integration tests for float and half_float range aggregations |
| 20_typed_keys.yml | Updated test data to use corrected float value instead of workaround |
| build.gradle | Skipped backward compatibility test containing old workaround |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Trying to parse infinite values into ints/longs throws. Understandably. | ||
| return value; | ||
| } | ||
| return type.parse(value, false).doubleValue(); |
There was a problem hiding this comment.
The indentation is inconsistent with the rest of the codebase. This line should use 12 spaces (or 3 levels of 4-space indentation) to align with standard Java formatting.
| return type.parse(value, false).doubleValue(); | |
| return type.parse(value, false).doubleValue(); |
| --- | ||
| "Float Endpoint Exclusive": | ||
| - skip: | ||
| version: " - 7.99.99" |
There was a problem hiding this comment.
The skip version '7.99.99' appears to be a placeholder. Consider using a more realistic version number or updating the reason to clarify when this test should actually run.
| version: " - 7.99.99" | |
| version: " - 7.15.99" |
| version: " - 7.99.99" | ||
| reason: Bug fixed in 7.16.0 (backport pending) |
There was a problem hiding this comment.
The skip version '7.99.99' appears to be a placeholder. Consider using a more realistic version number or updating the reason to clarify when this test should actually run.
2 participants
PR_018