Skip to content

Comments

Scale doubles to floats when necessary to match the field (#78344)#8

Open
MitchLewis930 wants to merge 1 commit intopr_018_beforefrom
pr_018_after
Open

Scale doubles to floats when necessary to match the field (#78344)#8
MitchLewis930 wants to merge 1 commit intopr_018_beforefrom
pr_018_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

PR_018


Note

Medium Risk
Touches core range aggregation query building and numeric parsing behavior; while narrowly scoped, it can subtly change bucket assignments at boundary values for numeric fields.

Overview
Range aggregations now reduce from/to bounds to the target numeric field’s stored precision (via a new NumberFieldType.reduceToStoredPrecision hook exposed through ValuesSourceConfig), preventing float/half-float vs double rounding mismatches at bucket boundaries.

Adds coverage for exclusive-endpoint behavior across double, float, and half_float in RangeAggregatorTests, updates REST YAML range tests to include float/half-float boundary cases, and adjusts a multisearch typed-keys test (plus skips it for v7 compat) to remove the prior workaround.

Written by Cursor Bugbot for commit b968fcb. This will update automatically on new commits. Configure here.

)

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
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

Double from = range.from;
Double to = range.to;
Double from = fixPrecision.applyAsDouble(range.from);
Double to = fixPrecision.applyAsDouble(range.to);
Copy link

Choose a reason for hiding this comment

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

Precision fix not applied to string-specified range values

Medium Severity

When range boundaries are specified as strings (e.g., {"from": "0.04"}) instead of numbers, the fixPrecision function is applied to range.from (which is Double.NEGATIVE_INFINITY for string-specified ranges), then the parsed string value overwrites from without the precision fix being applied to it. This means the float/double precision issue the PR aims to fix will persist for string-specified range values on float fields.

Fix in Cursor Fix in Web

return value;
}
return type.parse(value, false).doubleValue();
}
Copy link

Choose a reason for hiding this comment

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

Decimal range boundaries throw exceptions on integer fields

High Severity

The reduceToStoredPrecision method calls type.parse(value, false) with coerce=false for all numeric types. For INTEGER, LONG, SHORT, and BYTE fields, the parse method throws IllegalArgumentException when the value has a decimal part. This breaks valid use cases where range aggregations use decimal boundaries like {from: 0.5, to: 10.5} on integer fields, which worked before this change.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants