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
📝 WalkthroughWalkthroughThe changes implement precision-correction logic for range aggregations to align double values with their stored precision across numeric field types. A new method in NumberFieldMapper reduces doubles to stored precision, which is then applied to range bounds during aggregation queries. New float and half_float field types are added with corresponding YAML test cases and unit tests for exclusive endpoint behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java`:
- Line 27: RangeAggregationBuilder currently calls
DoubleUnaryOperator.applyAsDouble on Range.from/Range.to (and ignores string
overrides fromAsStr/toAsStr), which can NPE for null (unbounded) bounds and
skips precision adjustment for string-based inputs; update the code path that
applies precision so it null-checks before calling applyAsDouble and also
normalize/adjust precision when handling fromAsStr and toAsStr values
(convert/parse string bounds, apply the same DoubleUnaryOperator/precision
logic, and preserve null/unbounded semantics) so both numeric and string bounds
are handled null-safely and consistently; touch the logic around
RangeAggregationBuilder where range.from, range.to, fromAsStr, toAsStr and the
DoubleUnaryOperator are used to implement this.
In
`@server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java`:
- Line 16: The current change applies reduceToStoredPrecisionFunction() to all
NumberFieldType instances causing integer range aggs to throw; restrict the
precision-reduction to only floating point field types by checking the field's
type (e.g., FloatFieldType / DoubleFieldType or
NumberFieldType.isFloatingPoint()) before wrapping with
reduceToStoredPrecisionFunction(), leaving integer NumberFieldType handling
unchanged (so reduceToStoredPrecision() with coerce=false is not applied to
integer fields). Update the logic in ValuesSourceConfig where
reduceToStoredPrecisionFunction() is attached (and the related branches around
lines referenced) to guard with a floating-type check so only floating types get
the precision adjustment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rest-api-spec/build.gradlerest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/msearch/20_typed_keys.ymlrest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/40_range.ymlserver/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.javaserver/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.javaserver/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.javaserver/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java (1)
server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java (1)
NumberFieldMapper(65-1283)
🔇 Additional comments (7)
rest-api-spec/build.gradle (1)
85-86: LGTM!The skip entry is appropriately added for V7 compatibility testing. Since the YAML test now uses the corrected boundary value (
19.25instead of the workaround19.2499999), skipping this test in V7 compat mode is necessary because the old workaround behavior is no longer applicable.rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/msearch/20_typed_keys.yml (1)
67-67: LGTM!The boundary value change from
19.2499999to19.25correctly reflects the fix for scaling doubles to floats. The previous workaround value was needed because range endpoints were treated as doubles even when the field was a float, causing precision mismatches. With the fix in place, the cleaner boundary value can now be used directly.The test assertions remain correct:
- Bucket
*-19.25: captures17.3349and19.0517(2 docs)- Bucket
19.25-*: captures19.3717(1 doc)Also applies to: 81-81
server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java (1)
1043-1056: Precision-reduction helper looks solid.Clear intent and safe handling of infinities; aligns with the float/half-float casting behavior described in the PR.
rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/40_range.yml (2)
14-33: Mappings and bulk data updates look good.The added
floatandhalf_floatfields plus sample values give the tests the right coverage basis.
38-93: Nice coverage for exclusive endpoints on float/half_float.These cases validate the precision-fix behavior and mirror the existing double tests well.
server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java (2)
20-20: Exclusive-endpoint tests provide solid coverage.The new tests for DOUBLE/FLOAT/HALF_FLOAT directly validate the precision correction with boundary values. The NumericUtils usage is appropriate for the double case.
Also applies to: 103-164
237-237: Explicit Consumer casts are fine here.They make type inference unambiguous without changing behavior.
Also applies to: 268-268, 302-302, 336-336, 368-368
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.function.DoubleUnaryOperator; |
There was a problem hiding this comment.
Null-safe precision fix + apply it to string bounds.
range.from / range.to can be null for unbounded ranges, so applyAsDouble will NPE. Also, string overrides (fromAsStr/toAsStr) bypass the precision fix entirely. Both can break correctness for unbounded ranges and string-based inputs.
✅ Proposed fix
- Double from = fixPrecision.applyAsDouble(range.from);
- Double to = fixPrecision.applyAsDouble(range.to);
+ Double from = range.from == null ? null : fixPrecision.applyAsDouble(range.from);
+ Double to = range.to == null ? null : fixPrecision.applyAsDouble(range.to);
if (range.fromAsStr != null) {
- from = parser.parseDouble(range.fromAsStr, false, context::nowInMillis);
+ from = fixPrecision.applyAsDouble(parser.parseDouble(range.fromAsStr, false, context::nowInMillis));
}
if (range.toAsStr != null) {
- to = parser.parseDouble(range.toAsStr, false, context::nowInMillis);
+ to = fixPrecision.applyAsDouble(parser.parseDouble(range.toAsStr, false, context::nowInMillis));
}Also applies to: 156-175
🤖 Prompt for AI Agents
In
`@server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java`
at line 27, RangeAggregationBuilder currently calls
DoubleUnaryOperator.applyAsDouble on Range.from/Range.to (and ignores string
overrides fromAsStr/toAsStr), which can NPE for null (unbounded) bounds and
skips precision adjustment for string-based inputs; update the code path that
applies precision so it null-checks before calling applyAsDouble and also
normalize/adjust precision when handling fromAsStr and toAsStr values
(convert/parse string bounds, apply the same DoubleUnaryOperator/precision
logic, and preserve null/unbounded semantics) so both numeric and string bounds
are handled null-safely and consistently; touch the logic around
RangeAggregationBuilder where range.from, range.to, fromAsStr, toAsStr and the
DoubleUnaryOperator are used to implement this.
| import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; | ||
| import org.elasticsearch.index.fielddata.IndexNumericFieldData; | ||
| import org.elasticsearch.index.mapper.MappedFieldType; | ||
| import org.elasticsearch.index.mapper.NumberFieldMapper; |
There was a problem hiding this comment.
Guard precision reduction to floating types to avoid integer-range regressions.
reduceToStoredPrecisionFunction() applies to all NumberFieldTypes. For integer fields, reduceToStoredPrecision() parses with coerce=false, which will throw for decimal range bounds (previously accepted and rounded in range logic). Consider applying the precision fix only for floating types to avoid behavior changes on integer range aggs.
💡 Suggested fix
- public DoubleUnaryOperator reduceToStoredPrecisionFunction() {
- if (fieldContext() != null && fieldType() instanceof NumberFieldMapper.NumberFieldType) {
- return ((NumberFieldMapper.NumberFieldType) fieldType())::reduceToStoredPrecision;
- }
- return (value) -> value;
- }
+ public DoubleUnaryOperator reduceToStoredPrecisionFunction() {
+ if (fieldContext() != null && fieldType() instanceof NumberFieldMapper.NumberFieldType) {
+ NumberFieldMapper.NumberFieldType nft = (NumberFieldMapper.NumberFieldType) fieldType();
+ IndexNumericFieldData.NumericType nt = nft.numericType();
+ if (nt == IndexNumericFieldData.NumericType.FLOAT
+ || nt == IndexNumericFieldData.NumericType.HALF_FLOAT
+ || nt == IndexNumericFieldData.NumericType.DOUBLE) {
+ return nft::reduceToStoredPrecision;
+ }
+ }
+ return DoubleUnaryOperator.identity();
+ }Also applies to: 24-24, 326-335
🤖 Prompt for AI Agents
In
`@server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java`
at line 16, The current change applies reduceToStoredPrecisionFunction() to all
NumberFieldType instances causing integer range aggs to throw; restrict the
precision-reduction to only floating point field types by checking the field's
type (e.g., FloatFieldType / DoubleFieldType or
NumberFieldType.isFloatingPoint()) before wrapping with
reduceToStoredPrecisionFunction(), leaving integer NumberFieldType handling
unchanged (so reduceToStoredPrecision() with coerce=false is not applied to
integer fields). Update the logic in ValuesSourceConfig where
reduceToStoredPrecisionFunction() is attached (and the related branches around
lines referenced) to guard with a floating-type check so only floating types get
the precision adjustment.
2 participants
PR_018
Summary by CodeRabbit
New Features
Bug Fixes
Tests