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 29, 2026

User description

PR_018


PR Type

Bug fix


Description

  • Add reduceToStoredPrecision() method to NumberFieldType to adjust double values to field's precision

  • Apply precision reduction to range aggregation bounds before bucketing

  • Fix float/double precision mismatch causing values to fall in wrong buckets

  • Add comprehensive tests for float, double, and half_float range aggregations


Diagram Walkthrough

flowchart LR
  A["Range Bounds<br/>as Doubles"] -->|"reduceToStoredPrecision()"| B["Adjusted to Field<br/>Precision"]
  B -->|"Applied in<br/>RangeAggregationBuilder"| C["Correct Bucket<br/>Assignment"]
  D["NumberFieldType"] -->|"parse & convert"| E["Precision Reduction<br/>Function"]
  E -->|"via ValuesSourceConfig"| C
Loading

File Walkthrough

Relevant files
Enhancement
NumberFieldMapper.java
Add precision reduction method to NumberFieldType               

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

  • Add reduceToStoredPrecision(double) method to NumberFieldType class
  • Method reinterprets double values based on field's maximum precision
  • Handles infinite values by returning them unchanged
  • Parses value through field type and converts back to double to match
    field's precision
+15/-0   
ValuesSourceConfig.java
Add precision reduction function accessor                               

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java

  • Import NumberFieldMapper and DoubleUnaryOperator
  • Add reduceToStoredPrecisionFunction() method that returns precision
    adjustment function
  • Returns identity function for non-numeric fields or fields without
    NumberFieldType
  • Delegates to NumberFieldType.reduceToStoredPrecision() when applicable
+13/-0   
Bug fix
RangeAggregationBuilder.java
Apply precision reduction to range bounds                               

server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java

  • Import DoubleUnaryOperator for precision adjustment function
  • Retrieve precision reduction function from ValuesSourceConfig
  • Apply precision reduction to range bounds (from and to) before
    processing ranges
  • Preserves string-based range parsing logic
+9/-2     
Tests
RangeAggregatorTests.java
Add precision tests and clean up imports                                 

server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java

  • Remove unused imports (DirectoryReader, IndexReader, IndexSearcher,
    Directory)
  • Add NumericUtils import for double-to-sortable-long conversion
  • Add three new test methods for float, double, and half_float range
    endpoint exclusivity
  • Add explicit type casts (Consumer>) in existing
    tests for clarity
  • Remove duplicate testCase() helper method (moved to parent class)
+69/-36 
20_typed_keys.yml
Update float range test expectations                                         

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/msearch/20_typed_keys.yml

  • Update float range boundary from 19.2499999 to 19.25 to match
    precision-adjusted value
  • Update expected bucket key from "*-19.2499999" to "*-19.25" in
    assertions
+2/-2     
40_range.yml
Add float and half_float range aggregation tests                 

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/40_range.yml

  • Add float and half_float field type mappings to test index
  • Add float and half_float values to bulk test data
  • Add new "Float Endpoint Exclusive" test section validating float range
    bucketing
  • Add new "Half Float Endpoint Exclusive" test section validating
    half_float range bucketing
  • Both new tests verify correct bucket assignment with
    precision-adjusted boundaries
+63/-3   

)

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
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unhandled parse errors: The new reduceToStoredPrecision uses type.parse(value, false) without guarding against
parse failures (e.g., integer/long fields with fractional or out-of-range finite bounds),
which could turn previously-accepted range bounds into exceptions at runtime.

Referred Code
public double reduceToStoredPrecision(double value) {
    if (Double.isInfinite(value)) {
        // Trying to parse infinite values into ints/longs throws. Understandably.
        return value;
    }
  return type.parse(value, false).doubleValue();
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Potential user errors: If reduceToStoredPrecisionFunction() triggers mapper parsing exceptions for certain
numeric bounds, the resulting error messages may be exposed via API responses unless
higher layers sanitize them, which is not verifiable from this diff alone.

Referred Code
/*
 This will downgrade the precision of the range bounds to match the field's precision.  Fixes float/double issues, but not
 long/double issues.  See https://github.com/elastic/elasticsearch/issues/77033
 */
DoubleUnaryOperator fixPrecision = config.reduceToStoredPrecisionFunction();

// We need to call processRanges here so they are parsed before we make the decision of whether to cache the request
Range[] ranges = processRanges(range -> {
    DocValueFormat parser = config.format();
    assert parser != null;
    Double from = fixPrecision.applyAsDouble(range.from);
    Double to = fixPrecision.applyAsDouble(range.to);
    if (range.fromAsStr != null) {
        from = parser.parseDouble(range.fromAsStr, false, context::nowInMillis);
    }
    if (range.toAsStr != null) {
        to = parser.parseDouble(range.toAsStr, false, context::nowInMillis);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Validation behavior change: The new reduceToStoredPrecisionFunction() applies mapper parsing to user-provided numeric
range bounds for all NumberFieldTypes, which may change validation/acceptance rules for
external inputs (e.g., fractional bounds on integer fields) and needs confirmation against
intended API behavior.

Referred Code
public DoubleUnaryOperator reduceToStoredPrecisionFunction() {
    if (fieldContext() != null && fieldType() instanceof NumberFieldMapper.NumberFieldType) {
        return ((NumberFieldMapper.NumberFieldType) fieldType())::reduceToStoredPrecision;
    }
    return (value) -> value;
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard null range bounds

Add null checks for range.from and range.to before applying the
precision-reduction function to prevent a NullPointerException with unbounded
ranges.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java [166-167]

-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);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a NullPointerException that will occur for unbounded ranges, as range.from or range.to can be null and unboxing them for fixPrecision.applyAsDouble will crash. This is a critical bug fix.

Medium
General
Handle NaN in precision reduction

Update the reduceToStoredPrecision method to also handle NaN values, similar to
how infinite values are handled, to prevent parsing errors.

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java [1050-1056]

 public double reduceToStoredPrecision(double value) {
-    if (Double.isInfinite(value)) {
-        // Trying to parse infinite values into ints/longs throws. Understandably.
+    if (Double.isInfinite(value) || Double.isNaN(value)) {
         return value;
     }
     return type.parse(value, false).doubleValue();
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that NaN values are not handled in the new reduceToStoredPrecision method, which could lead to exceptions for integer-based fields. This improves the robustness of the new logic by handling a valid edge case.

Low
  • More

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.

2 participants