Skip to content

[FP16 PR-5] Enable half_float as datatype - #3552

Open
ManasviGoyal wants to merge 4 commits into
opensearch-project:feature/fp16-exact-searchfrom
ManasviGoyal:enable-half-float-datatype
Open

[FP16 PR-5] Enable half_float as datatype#3552
ManasviGoyal wants to merge 4 commits into
opensearch-project:feature/fp16-exact-searchfrom
ManasviGoyal:enable-half-float-datatype

Conversation

@ManasviGoyal

@ManasviGoyal ManasviGoyal commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Enables half_float as a new user-facing data_type on knn_vector fields, wiring the FP16 codec
landed in PR-1 through PR-4 into mapping, indexing, search, script scoring. Current supports 1x compression for HNSW Lucene and Flat (index.knn: true only)

Most changes (~1380 lines, 22 files) are just tests (unit + integration)

Related Issues

Resolves #3438 and #3498

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ManasviGoyal
enable half_float as datatype
d31e738 
Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
@ManasviGoyal ManasviGoyal changed the title enable half_float as datatype [FP16 PR-5] Enable half_float as datatype Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 02ab948)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections

🔀 Multiple PR themes

Sub-PR theme: Thread VectorDataType through codec format resolvers

Relevant files:

  • src/main/java/org/opensearch/knn/index/codec/KNN1040BasePerFieldKnnVectorsFormat.java
  • src/main/java/org/opensearch/knn/index/codec/KNN1040Codec/KNN1040PerFieldKnnVectorsFormat.java
  • src/main/java/org/opensearch/knn/index/codec/KnnVectorsFormatContext.java
  • src/main/java/org/opensearch/knn/index/codec/LuceneVectorsFormatType.java
  • src/main/java/org/opensearch/knn/index/engine/CodecFormatResolver.java
  • src/main/java/org/opensearch/knn/index/engine/faiss/FaissCodecFormatResolver.java
  • src/main/java/org/opensearch/knn/index/engine/lucene/LuceneCodecFormatResolver.java
Sub-PR theme: Method resolver validation for HALF_FLOAT compression rules

Relevant files:

  • src/main/java/org/opensearch/knn/index/engine/AbstractMethodResolver.java
  • src/main/java/org/opensearch/knn/index/engine/faiss/FaissMethodResolver.java
  • src/main/java/org/opensearch/knn/index/engine/lucene/LuceneFlatMethodResolver.java
  • src/main/java/org/opensearch/knn/index/engine/lucene/LuceneHNSWMethodResolver.java
  • src/main/java/org/opensearch/knn/index/engine/nmslib/NmslibMethodResolver.java
Sub-PR theme: Field strategy plumbing for half-float stored fields

Relevant files:

  • src/main/java/org/opensearch/knn/index/mapper/EngineFieldMapper.java
  • src/main/java/org/opensearch/knn/index/mapper/EngineFieldStrategy.java
  • src/main/java/org/opensearch/knn/index/mapper/LuceneFieldStrategy.java
  • src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java
⚡ Recommended focus areas for review

Incorrect error message

The IllegalArgumentException message in calculateArraySize still needs verification — the message includes half_float but doesn't include the unsupported vector data type from vectorDataType.getValue(), making debugging harder. Also confirm that when vectorDataType is null, this path is reached rather than throwing NPE earlier.

 
public static long calculateArraySize(int numVectors, int vectorLength, VectorDataType vectorDataType) {
    if (vectorDataType == VectorDataType.FLOAT) {
        return (long) numVectors * vectorLength * FLOAT_BYTE_SIZE;
    } else if (vectorDataType == VectorDataType.HALF_FLOAT) {
        return (long) numVectors * vectorLength * HALF_FLOAT_BYTE_SIZE;
    } else if (vectorDataType == VectorDataType.BINARY || vectorDataType == VectorDataType.BYTE) {
        return (long) numVectors * vectorLength;
    } else {
        throw new IllegalArgumentException(
            "Float, half_float, binary, and byte are the only supported vector data types for array size calculation."
        );
    }
}
Possible Issue

The validateModeAndCompression change loosens the compression rule to permit HALF_FLOAT but the mode rule stays float-only. A user configuring mode: on_disk with data_type: half_float will get the "Mode cannot be used for non-float32 data type" error, but a user configuring mode: in_memory (which is isConfigured=true) will also be rejected even though it's semantically a no-op for half_float. Consider whether Mode.IN_MEMORY should be tolerated or documented as unsupported.

 
private void validateModeAndCompression(KNNVectorFieldMapper.Builder builder, Version indexCreatedVersion) {
    VectorDataType vectorDataType = builder.vectorDataType.getValue();
    // TODO: Revisit half_float here once Lucene SQ on top of FP16 lands. Mode resolves to an
    // engine plus a compression level, and neither value means anything for half_float yet:
    // on_disk implies Faiss (half_float is Lucene-only) and in_memory implies x1, which is
    // already the default. It only becomes meaningful once x16 gives half_float a second tier.
    if (builder.mode.isConfigured() && vectorDataType != VectorDataType.FLOAT) {
        throw new MapperParsingException(
            String.format(Locale.ROOT, "Mode cannot be used for non-float32 data type for field %s", builder.name)
        );
    }
    if (builder.compressionLevel.isConfigured()
        && vectorDataType != VectorDataType.FLOAT
        && vectorDataType != VectorDataType.HALF_FLOAT) {
        throw new MapperParsingException(
            String.format(Locale.ROOT, "Compression cannot be used for non-float data type for field %s", builder.name)
        );
    }
Behavior change for FLOAT flat

Previously, a FLOAT flat method with an explicit non-x32 compression level compared against SUPPORTED_COMPRESSION_LEVELS (which was {x32}). Now the else branch rejects only when compressionLevel != defaultCompression (x32). Functionally equivalent for the current single-element set, but if SUPPORTED_COMPRESSION_LEVELS was intended to hold multiple levels in the future for FLOAT, that flexibility has been lost. Additionally, the removal of SUPPORTED_COMPRESSION_LEVELS as a constant may affect other code paths (verify no external references).

 
private CompressionLevel validateAndResolveCompressionLevel(KNNMethodConfigContext knnMethodConfigContext) {
    boolean isHalfFloat = VectorDataType.HALF_FLOAT == knnMethodConfigContext.getVectorDataType();
    // HALF_FLOAT isn't SQ, so it gets its own default instead of x32's rescore-triggering one.
    final CompressionLevel defaultCompression = isHalfFloat ? DEFAULT_COMPRESSION_HALF_FLOAT : DEFAULT_COMPRESSION;

    CompressionLevel compressionLevel = knnMethodConfigContext.getCompressionLevel();
    if (CompressionLevel.isConfigured(compressionLevel)) {
        if (isHalfFloat) {
            ValidationException validationException = validateCompressionSupported(
                compressionLevel,
                SUPPORTED_COMPRESSION_HALF_FLOAT,
                KNNEngine.LUCENE,
                knnMethodConfigContext.getVectorDataType(),
                null
            );
            if (validationException != null) {
                throw validationException;
            }
        } else if (compressionLevel != defaultCompression) {
            ValidationException validationException = new ValidationException();
            validationException.addValidationError(
                String.format(Locale.ROOT, "\"%s\" method only supports \"%s\" compression", METHOD_FLAT, defaultCompression.getName())
            );
            throw validationException;
        }
        return compressionLevel;
    }
    return defaultCompression;
}

@github-actions

github-actions Bot commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Code Suggestions ✨

Latest suggestions up to 02ab948

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid advertising doc values without writing them

Skipping the VectorField (binary DocValues) for HALF_FLOAT silently removes
doc-values-backed functionality (aggregations, sorting, script scoring via doc
values) unless another path adds equivalent doc values. If that is intentional,
ensure hasDocValues is forced false for HALF_FLOAT so the mapper does not advertise
doc values it never writes; otherwise HALF_FLOAT fields will appear to have doc
values but return nothing.

src/main/java/org/opensearch/knn/index/mapper/LuceneFieldStrategy.java [75-77]

 if (hasDocValues && vectorFieldType != null && vectorDataType != VectorDataType.HALF_FLOAT) {
     fields.add(new VectorField(name, array, vectorFieldType));
 }
+// NOTE: For HALF_FLOAT, ensure hasDocValues is not advertised as true elsewhere,
+// or add a half-float-specific doc values field here to avoid an inconsistent field state.
Suggestion importance[1-10]: 6

__

Why: Raises a legitimate concern about hasDocValues being advertised but not written for HALF_FLOAT, which could cause inconsistencies in doc-values-dependent operations. However, the improved code only adds a comment without a functional fix.

Low
General
Validate even byte length for half-float

bytesRef.length may be odd if the underlying bytes were produced by a different code
path; silently truncating via integer division could hide a
corruption/format-mismatch bug. Add an assertion or explicit check that
bytesRef.length % 2 == 0 to surface encoding mismatches early instead of returning a
shorter vector.

src/main/java/org/opensearch/knn/index/vectorvalues/VectorValueExtractorStrategy.java [141-146]

 private float[] getHalfFloatVectorFromByteRef(final BytesRef bytesRef) {
+    if ((bytesRef.length & 1) != 0) {
+        throw new IllegalStateException("Half-float byte length must be even, got " + bytesRef.length);
+    }
     int dimension = bytesRef.length / 2;
     float[] vector = new float[dimension];
     KNNVectorAsCollectionOfHalfFloatsSerializer.INSTANCE.byteToFloatArray(bytesRef.bytes, vector, dimension, bytesRef.offset);
     return vector;
 }
Suggestion importance[1-10]: 4

__

Why: Adds a reasonable defensive check for corruption detection, though such format mismatches should not occur in normal operation given controlled serialization paths.

Low
Verify BINARY array size calculation

For the BINARY case the size should reflect that BINARY dimension is 8x the byte
size (as noted elsewhere in this codebase), so numVectors * vectorLength may
over-count bytes. Verify the semantics of vectorLength for BINARY here to avoid
over-allocating/over-reporting memory used, which could trip circuit breakers.

src/main/java/org/opensearch/knn/index/codec/util/KNNCodecUtil.java [43-48]

 if (vectorDataType == VectorDataType.FLOAT) {
     return (long) numVectors * vectorLength * FLOAT_BYTE_SIZE;
 } else if (vectorDataType == VectorDataType.HALF_FLOAT) {
     return (long) numVectors * vectorLength * HALF_FLOAT_BYTE_SIZE;
-} else if (vectorDataType == VectorDataType.BINARY || vectorDataType == VectorDataType.BYTE) {
+} else if (vectorDataType == VectorDataType.BYTE) {
     return (long) numVectors * vectorLength;
+} else if (vectorDataType == VectorDataType.BINARY) {
+    return (long) numVectors * (vectorLength / 8);
Suggestion importance[1-10]: 3

__

Why: The BINARY calculation is pre-existing behavior not modified by this PR, and the suggestion assumes semantics of vectorLength without verification. Changing it could introduce regressions.

Low
Null-check vector before length access

Guard against a null vector to prevent a NullPointerException on vector.length,
matching the defensive style typical for field mapper utilities. Return early or
throw a clear IllegalArgumentException so callers get an actionable error instead of
an NPE deep in the codec path.

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java [88-92]

 public static StoredField createStoredFieldForHalfFloatVector(String name, float[] vector) {
+    if (vector == null) {
+        throw new IllegalArgumentException("vector must not be null for field [" + name + "]");
+    }
     byte[] output = new byte[vector.length * 2];
     KNNVectorAsCollectionOfHalfFloatsSerializer.INSTANCE.floatToByteArray(vector, output, vector.length);
     return new StoredField(name, output);
 }
Suggestion importance[1-10]: 2

__

Why: Defensive null check on internal utility method; callers typically validate vectors upstream, so the NPE risk is minor and adds minimal value.

Low

Previous suggestions

Suggestions up to commit 307aee6
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix always-true enum vs string comparison

The comparison vectorDataType.getValue() != VectorDataType.HALF_FLOAT compares a
String (from getValue()) to an enum, which is always true and effectively disables
this branch guard. Compare enum-to-enum directly using vectorDataType.getValue() to
the enum's string value, or use getVectorDataType()/enum comparison.

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java [310-312]

 if (originalParameters.getResolvedKnnMethodContext() == null
     && indexCreatedVersion.onOrAfter(Version.V_2_17_0)
     && vectorDataType.getValue() != VectorDataType.HALF_FLOAT) {
+    // Replace with proper enum comparison, e.g.:
+    // && originalParameters.getVectorDataType() != VectorDataType.HALF_FLOAT) {
Suggestion importance[1-10]: 6

__

Why: If vectorDataType.getValue() returns a String and is compared to a VectorDataType enum, the comparison would always be false, silently disabling the HALF_FLOAT exclusion. This is a valid concern worth verifying, though the improved_code is nearly identical to the existing code.

Low
Clarify enum comparison to prevent silent guard bypass

builder.vectorDataType.getValue() returns the underlying VectorDataType enum (via
lombok getValue() on a Parameter), but the naming is confusing and
error-prone—elsewhere in this file getValue() on a VectorDataType returns a String.
Confirm that this comparison actually resolves to enum equality (not String vs enum,
which is always false and would silently disable the guard). If ambiguous, extract
to a local VectorDataType dt = builder.vectorDataType.getValue(); and compare dt ==
VectorDataType.HALF_FLOAT.

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java [426-431]

-if (isKNNDisabled && builder.vectorDataType.getValue() == VectorDataType.HALF_FLOAT) {
+VectorDataType dt = builder.vectorDataType.getValue();
+if (isKNNDisabled && dt == VectorDataType.HALF_FLOAT) {
     throw new IllegalArgumentException(
         "HALF_FLOAT vector data type is not supported when index.knn is disabled. "
             + "Use method 'flat' with engine 'lucene' and index.knn enabled instead."
     );
 }
Suggestion importance[1-10]: 4

__

Why: Similar to suggestion 3, this asks to verify the enum comparison semantics. The concern is valid but largely a readability/verification suggestion rather than a definitive bug fix.

Low
General
Detect fp16 overflow via round-trip check

Comparing a float directly against FP16_MIN_VALUE/FP16_MAX_VALUE only checks
representable magnitude bounds, but values within [FP16_MIN_VALUE, FP16_MAX_VALUE]
may still lose precision or round to infinity when converted to fp16 (e.g., values
just below 65520 round up to +Inf). Consider validating by round-tripping through
Float.float16ToFloat(Float.floatToFloat16(value)) and rejecting results that become
infinite, to catch values that overflow fp16 during actual encoding.

src/main/java/org/opensearch/knn/common/KNNValidationUtil.java [79-82]

 public static void validateHalfFloatVectorValue(float value) {
     validateFloatVectorValue(value); // Check for NaN and Infinity
 
-    if (value < FP16_MIN_VALUE || value > FP16_MAX_VALUE) {
+    float roundTripped = Float.float16ToFloat(Float.floatToFloat16(value));
+    if (Float.isInfinite(roundTripped) || value < FP16_MIN_VALUE || value > FP16_MAX_VALUE) {
Suggestion importance[1-10]: 5

__

Why: Round-trip validation would catch values that round up to infinity during fp16 encoding, which is a legitimate edge case. However, the current bounds check is the standard approach and the improvement is marginal.

Low
Fail fast when doc values requested for half_float

When vectorDataType is HALF_FLOAT and neither hasDocValues/vectorFieldType nor
stored yields a field, the returned list contains only the
DerivedKnnFloatVectorField. This is fine, but if hasDocValues is true for a
HALF_FLOAT field, doc values are silently dropped—consider logging a warning or
rejecting hasDocValues=true at mapping time so callers relying on doc values (script
scoring, sorting) get a clear error instead of silent data loss.

src/main/java/org/opensearch/knn/index/mapper/LuceneFieldStrategy.java [75-84]

 if (hasDocValues && vectorFieldType != null && vectorDataType != VectorDataType.HALF_FLOAT) {
     fields.add(new VectorField(name, array, vectorFieldType));
+} else if (hasDocValues && vectorDataType == VectorDataType.HALF_FLOAT) {
+    throw new IllegalStateException("doc_values are not supported for half_float vectors");
 }
 if (stored) {
     if (vectorDataType == VectorDataType.HALF_FLOAT) {
         fields.add(createStoredFieldForHalfFloatVector(name, array));
     } else {
         fields.add(createStoredFieldForFloatVector(name, array));
     }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion to throw when hasDocValues=true for HALF_FLOAT is speculative; the PR handles this at the mapper level (e.g., FlatVectorFieldMapper exclusion), and silently skipping doc values here may be intentional. The impact is minor.

Low
Suggestions up to commit 4717367
CategorySuggestion                                                                                                                                    Impact
General
Verify doc-values access for half_float

Skipping the binary doc-values VectorField for HALF_FLOAT means script-scoring,
aggregations, and other doc-values-based consumers will not find vector values under
this field. If HALF_FLOAT is expected to support script scoring (as indicated by the
tests in KNNScoringSpaceUtilTests and KNNScoringUtil changes), you should either add
an FP16-encoded doc-values field here or verify that the KnnFloatVectorField alone
satisfies all read paths (e.g., KNNVectorScriptDocValues).

src/main/java/org/opensearch/knn/index/mapper/LuceneFieldStrategy.java [73-77]

 final List<Field> fields = new ArrayList<>();
 fields.add(new DerivedKnnFloatVectorField(name, array, fieldType, isDerivedSourceEnabled));
 if (hasDocValues && vectorFieldType != null && vectorDataType != VectorDataType.HALF_FLOAT) {
     fields.add(new VectorField(name, array, vectorFieldType));
 }
+// TODO: verify script-scoring/doc-values consumers work for HALF_FLOAT without a binary doc-values field
Suggestion importance[1-10]: 5

__

Why: This raises a legitimate concern about whether script-scoring and doc-values consumers will work correctly for HALF_FLOAT without a binary doc-values field. However, it only asks for verification without proposing a concrete fix, and the improved_code just adds a TODO comment.

Low
Consider overflow-safe multiplication

The expression (long) numVectors * vectorLength * FLOAT_BYTE_SIZE still risks int
overflow because numVectors * vectorLength is evaluated as int before being
multiplied by the long-cast operand order. Actually, (long) numVectors casts first,
so the multiplication chain becomes long throughout — this is correct. However,
verify the same holds for HALF_FLOAT (it does). No change needed if intent is clear;
else use explicit Math.multiplyExact to catch pathological overflow.

src/main/java/org/opensearch/knn/index/codec/util/KNNCodecUtil.java [43-48]

+if (vectorDataType == VectorDataType.FLOAT) {
+    return (long) numVectors * vectorLength * FLOAT_BYTE_SIZE;
+} else if (vectorDataType == VectorDataType.HALF_FLOAT) {
+    return (long) numVectors * vectorLength * HALF_FLOAT_BYTE_SIZE;
+} else if (vectorDataType == VectorDataType.BINARY || vectorDataType == VectorDataType.BYTE) {
+    return (long) numVectors * vectorLength;
+} else {
 
-
Suggestion importance[1-10]: 2

__

Why: The suggestion acknowledges the existing code is already correct (the (long) cast promotes the entire expression to long). The improved_code is identical to existing_code, providing no actual improvement.

Low
Possible issue
Fix incorrect type comparison in condition

The condition vectorDataType.getValue() != VectorDataType.HALF_FLOAT compares a
String (return of getValue()) against a VectorDataType enum via !=, which will
always evaluate to true (reference inequality between different types). This defeats
the intended exclusion of HALF_FLOAT from the FlatVectorFieldMapper path. Use
vectorDataType.getValue() != VectorDataType.HALF_FLOAT should be
vectorDataType.get() != VectorDataType.HALF_FLOAT or compare enums directly.

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java [310-312]

 if (originalParameters.getResolvedKnnMethodContext() == null
     && indexCreatedVersion.onOrAfter(Version.V_2_17_0)
-    && vectorDataType.getValue() != VectorDataType.HALF_FLOAT) {
+    && vectorDataType.get() != VectorDataType.HALF_FLOAT) {
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a potential concern about getValue() returning a String vs enum, but without full context of the vectorDataType Parameter's type it's speculative. The improved_code is essentially the same as existing_code with only a method rename that may not be correct.

Low
Verify enum vs string comparison correctness

Same type-mismatch bug as above: builder.vectorDataType.getValue() returns the
VectorDataType enum value from the Parameter (not a String — since the builder
Parameter holds VectorDataType), but verify carefully. If getValue() here returns a
VectorDataType, this is correct; if it returns String, the comparison silently
always evaluates false and the guard becomes dead code. Please double-check the
Parameter's value type and either use .get() or compare against the string name
consistently.

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java [426-431]

+if (isKNNDisabled && builder.vectorDataType.getValue() == VectorDataType.HALF_FLOAT) {
+    throw new IllegalArgumentException(
+        "HALF_FLOAT vector data type is not supported when index.knn is disabled. "
+            + "Use method 'flat' with engine 'lucene' and index.knn enabled instead."
+    );
+}
 
-
Suggestion importance[1-10]: 3

__

Why: Similar to suggestion 2, this asks for verification of the comparison. The improved_code is identical to existing_code, and it's only a verification request without a concrete fix.

Low
Suggestions up to commit d31e738
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid integer overflow in size calculation

The multiplications are performed in int arithmetic and only widened to long on
return. For large indices (numVectors * vectorLength * 4 exceeding
Integer.MAX_VALUE) this silently overflows. Cast one operand to long before
multiplying, as done idiomatically for sizing calculations.

src/main/java/org/opensearch/knn/index/codec/util/KNNCodecUtil.java [43-48]

 if (vectorDataType == VectorDataType.FLOAT) {
-    return numVectors * vectorLength * FLOAT_BYTE_SIZE;
+    return (long) numVectors * vectorLength * FLOAT_BYTE_SIZE;
 } else if (vectorDataType == VectorDataType.HALF_FLOAT) {
-    return numVectors * vectorLength * HALF_FLOAT_BYTE_SIZE;
+    return (long) numVectors * vectorLength * HALF_FLOAT_BYTE_SIZE;
 } else if (vectorDataType == VectorDataType.BINARY || vectorDataType == VectorDataType.BYTE) {
-    return numVectors * vectorLength;
+    return (long) numVectors * vectorLength;
 } else {
Suggestion importance[1-10]: 7

__

Why: Valid concern: the multiplications are done in int arithmetic and returned as long, which can overflow for large indices. However, this issue existed prior to the PR for the FLOAT case, so it's a pre-existing bug that the suggestion correctly identifies extending to HALF_FLOAT.

Medium
General
Verify HALF_FLOAT read paths without doc values

Suppressing doc values for HALF_FLOAT here means script scoring, exact search, and
other paths that rely on binary doc values will silently return no values for
HALF_FLOAT fields. Confirm that all HALF_FLOAT read paths (e.g.
KNNVectorDVLeafFieldData, KNNVectorScriptDocValues, script scoring tests) can
operate off the KnnFloatVector storage rather than doc values, otherwise this will
produce empty/zero results at query time.

src/main/java/org/opensearch/knn/index/mapper/LuceneFieldStrategy.java [75]

+final List<Field> fields = new ArrayList<>();
+fields.add(new DerivedKnnFloatVectorField(name, array, fieldType, isDerivedSourceEnabled));
+if (hasDocValues && vectorFieldType != null && vectorDataType != VectorDataType.HALF_FLOAT) {
+    fields.add(new VectorField(name, array, vectorFieldType));
+}
 
-
Suggestion importance[1-10]: 4

__

Why: The improved_code is identical to the existing_code, so the suggestion only asks the user to verify behavior rather than proposing an actual change. Per guidelines, verification-only suggestions receive lower scores.

Low
Verify FP16 min constant semantics

validateFloatVectorValue only rejects NaN/Infinity, but values in the finite-float
range that are outside [FP16_MIN_VALUE, FP16_MAX_VALUE] but round to ±Infinity in
FP16 (e.g. 70000f) are being flagged here. However, subnormal-underflow silently
rounds to 0 which may be acceptable — confirm that FP16_MIN_VALUE reflects the
actual negative max (-65504) rather than the smallest positive normal, otherwise
valid negative values will be rejected.

src/main/java/org/opensearch/knn/common/KNNValidationUtil.java [82-92]

 if (value < FP16_MIN_VALUE || value > FP16_MAX_VALUE) {
     throw new IllegalArgumentException(
         String.format(
             Locale.ROOT,
-            "[%s] field was set as HALF_FLOAT in index mapping. But, KNN vector values are not within in the half_float range [%f, %f]",
+            "[%s] field was set as HALF_FLOAT in index mapping. But, KNN vector value [%f] is not within the half_float range [%f, %f]",
             VECTOR_DATA_TYPE_FIELD,
+            value,
             FP16_MIN_VALUE,
             FP16_MAX_VALUE
         )
     );
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion mainly asks for verification of the FP16_MIN_VALUE constant semantics and offers a minor improvement to include the offending value in the error message. Low-to-moderate impact.

Low

@ManasviGoyal
minor fixes in java doc
4717367 
Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 4717367

@ManasviGoyal
ManasviGoyal marked this pull request as ready for review September 4, 2026 18:42
@ManasviGoyal
Include vector data type in compression validation errors
307aee6 
Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 307aee6

@ManasviGoyal
Add half_float retrieval ITs and clarify data type comparisons in the…
02ab948 
… mapper

Signed-off-by: Manasvi Goyal <mg.manasvi@gmail.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 02ab948

@codecov

codecov Bot commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.79310% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.31%. Comparing base (f0bcf68) to head (02ab948).

Files with missing lines Patch % Lines
...nsearch/knn/index/mapper/KNNVectorFieldMapper.java 55.55% 3 Missing and 5 partials ⚠️
...dex/vectorvalues/VectorValueExtractorStrategy.java 25.00% 5 Missing and 1 partial ⚠️
...g/opensearch/knn/plugin/script/KNNScoringUtil.java 0.00% 0 Missing and 6 partials ⚠️
...earch/knn/index/engine/AbstractMethodResolver.java 50.00% 3 Missing and 1 partial ⚠️
...ch/knn/index/engine/faiss/AbstractFaissMethod.java 0.00% 4 Missing ⚠️
.../java/org/opensearch/knn/index/VectorDataType.java 57.14% 3 Missing ⚠️
...ensearch/knn/index/mapper/LuceneFieldStrategy.java 25.00% 1 Missing and 2 partials ⚠️
...opensearch/knn/index/KNNVectorScriptDocValues.java 0.00% 0 Missing and 1 partial ⚠️
...search/knn/index/mapper/PerDimensionValidator.java 75.00% 1 Missing ⚠️
...rg/opensearch/knn/index/query/KNNQueryFactory.java 50.00% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files 
@@                      Coverage Diff                      @@
##             feature/fp16-exact-search    #3552    +/-   ##
=============================================================
  Coverage                        84.31%   84.31%            
- Complexity                        4656     4692    +36     
=============================================================
  Files                              469      470     +1     
  Lines                            16623    16727   +104     
  Branches                          2172     2191    +19     
=============================================================
+ Hits                             14016    14104    +88     
- Misses                            1801     1812    +11     
- Partials                           806      811     +5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Reviewers

@navneet1v navneet1v navneet1v approved these changes
@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner
@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner
@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner
@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner
@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner
@luyuncheng luyuncheng Awaiting requested review from luyuncheng luyuncheng is a code owner
@shatejas shatejas Awaiting requested review from shatejas shatejas is a code owner
@0ctopus13prime 0ctopus13prime Awaiting requested review from 0ctopus13prime 0ctopus13prime is a code owner
@Vikasht34 Vikasht34 Awaiting requested review from Vikasht34 Vikasht34 is a code owner
@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

skip-changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ManasviGoyal @navneet1v