Skip to content

[Indexing] Add support of "extra fields" outside _source indexing#20635

Open
laminelam wants to merge 3 commits intoopensearch-project:mainfrom
laminelam:feature/extra_fields_36
Open

[Indexing] Add support of "extra fields" outside _source indexing#20635
laminelam wants to merge 3 commits intoopensearch-project:mainfrom
laminelam:feature/extra_fields_36

Conversation

@laminelam
Copy link
Contributor

This PR introduces ExtraFieldValues, a side-channel mechanism for supplying field values outside of _source during indexing.

This change is part of a broader effort to enhance gRPC indexing, enabling primitive vector values to be indexed as non-_source data for improved ingestion performance.

Key points:

  • Adds ExtraFieldValues support to IndexRequest and UpdateRequest (doc and upsert paths).

  • Enables mappers to consume values directly, bypassing JSON parsing.

  • Currently supports float arrays only (primitive and packed little-endian representations).

  • Transport serialization is version-gated for backward compatibility.

  • For now, only doc-based updates and upserts are supported (script not supported).

Benchmark

Benchmarked ingest throughput across increasing vector dimensions using four representations:

  • JSON (float array)
  • JSON (binary/base64)
  • SMILE
  • ExtraFieldValues (non _source side-channel)

Across all tested dimensions:

JSON float arrays show the lowest throughput and degrade rapidly as vector size increases.

JSON (binary/base64) performs significantly better than float arrays.

SMILE and JSON(base64) are generally comparable, with SMILE often providing significant improvements at higher dimensions.

ExtraFieldValues consistently provides the highest throughput and scales best with increasing vector size.

At higher dimensions (e.g., 1024+), ExtraFieldValues shows a clear and consistent advantage over the _source-based approaches, achieving up to 60× higher throughput compared to JSON arrays.

These results indicate that bypassing _source parsing for large vector payloads can substantially improve ingest throughput.

Note: ExtraFieldValues was tested with the PackedFloatArray option (packed little-endian floats)

extra_fields

Ingest Throughput (baseline = JSON float array)

docs/sec (X× faster with json array as base)

Dim JSON (array) JSON (base64/binary) SMILE (binary) ExtraFieldValues
8 19,619 (1.0×) 30,817 (1.57×) 27,405 (1.40×) 26,793 (1.37×)
64 6,572 (1.0×) 23,637 (3.60×) 28,010 (4.26×) 26,284 (4.00×)
128 3,649 (1.0×) 19,428 (5.32×) 14,375 (3.94×) 25,899 (7.10×)
512 1,132 (1.0×) 9,723 (8.59×) 12,532 (11.07×) 20,830 (18.40×)
768 778 (1.0×) 6,883 (8.85×) 10,424 (13.40×) 18,443 (23.70×)
1024 591 (1.0×) 5,630 (9.53×) 8,453 (14.31×) 17,663 (29.90×)
1536 399 (1.0×) 3,951 (9.89×) 6,061 (15.18×) 15,398 (38.56×)
2048 299 (1.0×) 3,034 (10.14×) 4,659 (15.56×) 14,887 (49.74×)
3072 196 (1.0×) 2,060 (10.50×) 3,058 (15.59×) 11,840 (60.34×)

Related Issues

#19638

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 15, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces comprehensive support for storing extra field values (fields outside _source) across OpenSearch's indexing and update paths. It adds typed value containers (BYTES, FLOAT_ARRAY), integrates them into request objects, and implements parsing logic to validate and store these values via the mapper framework.

Changes

Cohort / File(s) Summary
Extra Field Value Type System
server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValue.java, BytesValue.java, FloatArrayValue.java, PackedFloatArray.java, PrimitiveFloatArray.java
Introduces sealed interface with nested Type enum (BYTES, FLOAT_ARRAY) and streaming protocol. Provides implementations for bytes data and float array storage (packed and primitive), with lazy decoding and caching for packed arrays.
Extra Field Values Container
server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValues.java
Immutable map-based container for field path to ExtraFieldValue mapping; supports serialization/deserialization and provides query accessors.
Request Path Integration
server/src/main/java/org/opensearch/action/index/IndexRequest.java, update/UpdateRequest.java, update/UpdateHelper.java, bulk/TransportShardBulkAction.java, SourceToParse.java
Adds extraFieldValues field and accessors to IndexRequest; adds docExtraFieldValues and upsertExtraFieldValues setters to UpdateRequest with validation against scripted updates; propagates values through UpdateHelper and bulk processing; extends SourceToParse with version-aware serialization.
Parsing and Mapper Framework
server/src/main/java/org/opensearch/index/mapper/DocumentParser.java, FieldMapper.java
Adds applyExtraFieldValues processing logic to DocumentParser with path validation and mapper support checks; introduces supportsExtraFieldValues() hook in FieldMapper for declarative support.
Unit Tests - Value Types
server/src/test/java/org/opensearch/index/mapper/extrasource/BytesValueTests.java, ExtraFieldValueTests.java, ExtraFieldValuesTests.java, PackedFloatArrayTests.java, PrimitiveFloatArrayTests.java
Validates serialization round-trips, immutability, lazy decoding with caching, bounds checking, and type dispatch for all value implementations.
Unit Tests - Ingestion & Validation
server/src/test/java/org/opensearch/action/update/UpdateRequestTests.java, index/mapper/extrasource/ExtraFieldValuesIngestTests.java
Tests parsing integration, rejection of extra field values with scripted updates, handling of unmapped fields, and root/nested field paths.
Integration Tests
server/src/internalClusterTest/java/org/opensearch/index/mapper/ExtraFieldValuesRequestIT.java, update/UpdateIT.java
End-to-end validation of index, update, upsert, and bulk operations carrying extra field values; verifies stored field metadata preservation.
Test Infrastructure
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesMapperPlugin.java, ExtraTestFieldMapper.java
Custom mapper plugin and field type for test environments; ExtraTestFieldMapper parses ExtraFieldValue implementations and stores type/dimension metadata.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Request as Index/Update/Bulk<br/>Request
    participant Transport as Transport<br/>ShardBulkAction
    participant Parser as Document<br/>Parser
    participant FieldMapper as Field<br/>Mapper
    participant Storage as Lucene<br/>Document

    Client->>Request: create with<br/>extraFieldValues()
    Request->>Transport: process (primary)
    Transport->>Parser: call with<br/>SourceToParse
    Parser->>Parser: applyExtraFieldValues()
    loop for each extra field
        Parser->>Parser: validate path &<br/>nested parents
        Parser->>FieldMapper: resolve mapper
        FieldMapper->>FieldMapper: supportsExtraFieldValues()?
        alt supported
            FieldMapper->>FieldMapper: parseCreateField()
            FieldMapper->>Storage: store field +<br/>metadata (_type, _dim)
        else not supported
            FieldMapper->>Parser: throw validation error
        end
    end
    Parser->>Storage: finalize document
    Transport->>Transport: replicate (replica)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • opensearch-project/OpenSearch#19958: Modifies DocumentParser's internalParseDocument flow for disabling nested objects/flattening, similar interaction pattern with core parsing logic.

Suggested labels

enhancement, Indexing & Search, Search:Performance

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.42% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main change: adding support for 'extra fields' outside _source during indexing, which aligns with the core purpose of the changeset.
Description check ✅ Passed The PR description comprehensively covers the change objectives, includes detailed context about ExtraFieldValues functionality, provides benchmark results showing performance improvements, and completes all checklist items appropriately.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@server/src/main/java/org/opensearch/action/update/UpdateRequest.java`:
- Around line 724-733: The Javadoc for UpdateRequest.docExtraFieldValues has a
typo ("extral") and should be corrected to "extra"; update the method comment
block above public UpdateRequest docExtraFieldValues(ExtraFieldValues values) to
read "Sets extra field values for the partial document update" and adjust any
related wording (e.g., "These values...") while keeping the rest of the
documentation and references to ExtraFieldValues,
safeDoc().extraFieldValues(values), and ExtraFieldValues#EMPTY unchanged.
🧹 Nitpick comments (5)
server/src/test/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArrayTests.java (1)

54-57: Minor: Duplicate assertion.

Line 56 duplicates the assertion at line 54 (read.dimension() is checked twice for vals.length).

🔧 Remove duplicate assertion
         assertThat(read.dimension(), is(vals.length));
         assertThat(read.isPackedLE(), is(false));
-        assertThat(read.dimension(), is(vals.length));
         assertThat(read.asFloatArray(), equalTo(vals));
server/src/main/java/org/opensearch/index/mapper/DocumentParser.java (1)

153-189: Resolve or track the EFV/JSON exclusivity TODO. The TODO hints at a missing validation rule; leaving both JSON and EFV populated could create ambiguous precedence. Consider enforcing it here or opening a follow‑up.

If you want, I can draft the validation or open a tracking issue.

server/src/main/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArray.java (2)

60-65: Add braces around the for-loop body for consistency.

The loop body at lines 62-63 lacks braces, which is inconsistent with the style in writePayloadTo (lines 55-57).

🔧 Suggested fix
     static PrimitiveFloatArray readBodyFrom(StreamInput in, int dim) throws IOException {
         final float[] v = new float[dim];
-        for (int j = 0; j < dim; j++)
-            v[j] = in.readFloat();
+        for (int j = 0; j < dim; j++) {
+            v[j] = in.readFloat();
+        }
         return new PrimitiveFloatArray(v);
     }

24-26: Consider null validation in constructor.

The constructor accepts the array without null-checking. If a null array is passed, subsequent calls to dimension() or get() will throw NPE. Since this is a public class, defensive validation may be warranted.

🛡️ Suggested fix
     public PrimitiveFloatArray(float[] v) {
+        java.util.Objects.requireNonNull(v, "v must not be null");
         this.v = v;
     }
server/src/test/java/org/opensearch/index/mapper/extrasource/PackedFloatArrayTests.java (1)

159-163: Remove unused helper method setPrivateField.

This helper method is defined but never used in any test. Consider removing it to avoid dead code, or add a test that requires it.

🧹 Suggested fix
-    private static void setPrivateField(Object target, String fieldName, Object value) throws Exception {
-        Field f = target.getClass().getDeclaredField(fieldName);
-        f.setAccessible(true);
-        f.set(target, value);
-    }
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c244c0 and 5d4649e.

📒 Files selected for processing (24)
  • server/src/internalClusterTest/java/org/opensearch/index/mapper/ExtraFieldValuesRequestIT.java
  • server/src/internalClusterTest/java/org/opensearch/update/UpdateIT.java
  • server/src/main/java/org/opensearch/action/bulk/TransportShardBulkAction.java
  • server/src/main/java/org/opensearch/action/index/IndexRequest.java
  • server/src/main/java/org/opensearch/action/update/UpdateHelper.java
  • server/src/main/java/org/opensearch/action/update/UpdateRequest.java
  • server/src/main/java/org/opensearch/index/mapper/DocumentParser.java
  • server/src/main/java/org/opensearch/index/mapper/FieldMapper.java
  • server/src/main/java/org/opensearch/index/mapper/SourceToParse.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/BytesValue.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValue.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValues.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/FloatArrayValue.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/PackedFloatArray.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArray.java
  • server/src/test/java/org/opensearch/action/update/UpdateRequestTests.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/BytesValueTests.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValueTests.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesIngestTests.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesMapperPlugin.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesTests.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraTestFieldMapper.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/PackedFloatArrayTests.java
  • server/src/test/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArrayTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:40:27.167Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.

Applied to files:

  • server/src/main/java/org/opensearch/action/index/IndexRequest.java
  • server/src/main/java/org/opensearch/index/mapper/DocumentParser.java
  • server/src/main/java/org/opensearch/action/update/UpdateRequest.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValues.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/FloatArrayValue.java
  • server/src/main/java/org/opensearch/action/update/UpdateHelper.java
  • server/src/main/java/org/opensearch/index/mapper/SourceToParse.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/BytesValue.java
  • server/src/main/java/org/opensearch/index/mapper/FieldMapper.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArray.java
  • server/src/main/java/org/opensearch/action/bulk/TransportShardBulkAction.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValue.java
  • server/src/main/java/org/opensearch/index/mapper/extrasource/PackedFloatArray.java
🧬 Code graph analysis (8)
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValueTests.java (1)
libs/core/src/main/java/org/opensearch/core/common/bytes/BytesArray.java (1)
  • BytesArray (48-151)
server/src/test/java/org/opensearch/index/mapper/extrasource/BytesValueTests.java (1)
libs/core/src/main/java/org/opensearch/core/common/bytes/BytesArray.java (1)
  • BytesArray (48-151)
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesTests.java (1)
libs/core/src/main/java/org/opensearch/core/common/bytes/BytesArray.java (1)
  • BytesArray (48-151)
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraTestFieldMapper.java (1)
server/src/main/java/org/opensearch/index/mapper/FieldMapper.java (3)
  • Builder (85-214)
  • Builder (740-793)
  • Builder (894-913)
server/src/internalClusterTest/java/org/opensearch/index/mapper/ExtraFieldValuesRequestIT.java (3)
libs/core/src/main/java/org/opensearch/core/common/bytes/BytesArray.java (1)
  • BytesArray (48-151)
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesMapperPlugin.java (1)
  • ExtraFieldValuesMapperPlugin (19-28)
server/src/main/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArray.java (1)
  • PrimitiveFloatArray (21-66)
server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValue.java (1)
server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValues.java (1)
  • ExperimentalApi (20-53)
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesMapperPlugin.java (1)
server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraTestFieldMapper.java (1)
  • ExtraTestFieldMapper (26-106)
server/src/main/java/org/opensearch/index/mapper/extrasource/PackedFloatArray.java (1)
libs/core/src/main/java/org/opensearch/core/common/bytes/BytesArray.java (1)
  • BytesArray (48-151)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
  • GitHub Check: gradle-check
  • GitHub Check: detect-breaking-change
  • GitHub Check: precommit (25, windows-latest)
  • GitHub Check: precommit (21, macos-15)
  • GitHub Check: precommit (25, macos-15-intel)
  • GitHub Check: precommit (21, windows-2025, true)
  • GitHub Check: precommit (25, ubuntu-24.04-arm)
  • GitHub Check: precommit (25, macos-15)
  • GitHub Check: precommit (21, ubuntu-24.04-arm)
  • GitHub Check: Analyze (java)
  • GitHub Check: precommit (25, ubuntu-latest)
  • GitHub Check: precommit (21, windows-latest)
  • GitHub Check: precommit (21, ubuntu-latest)
  • GitHub Check: precommit (21, macos-15-intel)
  • GitHub Check: assemble (21, ubuntu-latest)
  • GitHub Check: assemble (21, ubuntu-24.04-arm)
  • GitHub Check: assemble (21, windows-latest)
  • GitHub Check: assemble (25, ubuntu-latest)
  • GitHub Check: assemble (25, windows-latest)
  • GitHub Check: assemble (25, ubuntu-24.04-arm)
  • GitHub Check: Mend Security Check
🔇 Additional comments (35)
server/src/main/java/org/opensearch/action/update/UpdateHelper.java (1)

254-254: LGTM - Correct propagation of ExtraFieldValues in doc-based update path.

This correctly forwards extraFieldValues from the incoming doc request to the final IndexRequest, ensuring EFV data flows through the update pipeline for non-script updates.

server/src/main/java/org/opensearch/index/mapper/FieldMapper.java (2)

336-341: LGTM - Clean opt-in hook for ExtraFieldValues support.

The method follows the established pattern in FieldMapper (similar to parsesArrayValue()), providing a sensible default of false that requires mappers to explicitly opt-in for EFV support.


50-50: The ExtraFieldValues import is used in the Javadoc comment at line 337 ({@link ExtraFieldValues}). The import is necessary for proper Javadoc link resolution and should be retained.

Likely an incorrect or invalid review comment.

server/src/test/java/org/opensearch/action/update/UpdateRequestTests.java (1)

653-666: LGTM - Good test coverage for script + ExtraFieldValues validation.

The test correctly validates that combining a script with ExtraFieldValues on the upsert path triggers the expected validation error. The test structure is consistent with other validation tests in this file.

server/src/main/java/org/opensearch/action/bulk/TransportShardBulkAction.java (2)

665-672: LGTM - Correct EFV propagation on primary path.

The SourceToParse construction now includes extraFieldValues() from the index request, enabling the parsing layer to consume extra field values during primary shard indexing.


918-919: LGTM - Consistent EFV propagation on replica path.

The replica path mirrors the primary path change, ensuring extraFieldValues are consistently available during replica shard indexing. This symmetry is important for correct replication behavior.

server/src/internalClusterTest/java/org/opensearch/update/UpdateIT.java (1)

550-564: LGTM - Good integration test for script + ExtraFieldValues rejection.

This integration test complements the unit test in UpdateRequestTests by validating the same behavior through the full client API stack. The test follows the established patterns in this file (using indexOrAlias(), expectThrows, etc.).

server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValueTests.java (1)

21-69: LGTM - Comprehensive serialization tests for ExtraFieldValue types.

The tests cover essential scenarios:

  • BytesValue round-trip with content verification
  • FloatArrayValue round-trip with dimension and value verification
  • Error handling for unknown type IDs

Good coverage of the serialization contract.

server/src/main/java/org/opensearch/index/mapper/extrasource/BytesValue.java (1)

17-35: LGTM - Clean record implementation for BytesValue.

The record pattern is appropriate for this immutable value type. The implementation is minimal and focused with proper visibility for readBodyFrom.

One consideration: the record accepts a potentially null BytesReference without validation. If null safety is a concern, consider adding a compact constructor with a null check. However, this may not be necessary if callers are expected to validate inputs.

server/src/test/java/org/opensearch/index/mapper/extrasource/PrimitiveFloatArrayTests.java (1)

20-75: LGTM - Thorough test coverage for PrimitiveFloatArray.

The tests comprehensively cover:

  • Basic accessors (dimension, get, asFloatArray, isPackedLE)
  • Error handling (packedBytes() throws on unpacked instance)
  • Serialization round-trip
  • Empty array edge case

Good alignment with the PrimitiveFloatArray implementation.

server/src/main/java/org/opensearch/action/update/UpdateRequest.java (3)

63-63: LGTM. ExtraFieldValues import aligns with the new API usage.


239-245: Good guard against scripted EFV usage. Prevents unsupported script paths from accepting extra field values.


821-829: LGTM. Upsert EFV setter mirrors the doc path cleanly.

server/src/test/java/org/opensearch/index/mapper/extrasource/BytesValueTests.java (1)

1-82: Solid round‑trip coverage for BytesValue. Exercises BytesArray, composite, and dispatch paths.

server/src/main/java/org/opensearch/index/mapper/DocumentParser.java (3)

50-51: LGTM. EFV imports are consistent with the new parsing flow.


146-146: LGTM. Applying EFV before metadata post-parse keeps the pipeline ordering consistent.


191-205: LGTM. Nested‑parent guard is clear and appropriately scoped.

server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesTests.java (1)

1-89: Nice coverage of immutability and stream round‑trip.

server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesMapperPlugin.java (1)

1-27: LGTM. Clear and minimal mapper plugin registration for tests.

server/src/main/java/org/opensearch/action/index/IndexRequest.java (5)

65-65: LGTM. EFV import matches the new request field.


116-116: LGTM. Defaulting to ExtraFieldValues.EMPTY avoids null checks.


158-160: LGTM. Version‑gated EFV deserialization looks correct.


504-519: LGTM. Setter null‑resets to EMPTY and the getter keeps the API clean.


702-704: LGTM. Version‑gated EFV serialization is consistent with the read path.

server/src/main/java/org/opensearch/index/mapper/SourceToParse.java (3)

40-40: LGTM. EFV import is consistent with new constructor overloads.


62-84: LGTM. Constructor chaining keeps backward compatibility and defaults EFV to EMPTY.


110-112: LGTM. Simple, clear EFV accessor.

server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValues.java (1)

1-51: LGTM. Clean immutable container with streamable support.

server/src/main/java/org/opensearch/index/mapper/extrasource/FloatArrayValue.java (1)

17-68: LGTM!

The interface design is clean with a well-defined contract:

  • Proper sealed interface extending ExtraFieldValue
  • Symmetric read/write protocol with writeBodyTo/readBodyFrom
  • Clear separation between header metadata (packedLE flag, dimension) and payload serialization via writePayloadTo
server/src/internalClusterTest/java/org/opensearch/index/mapper/ExtraFieldValuesRequestIT.java (1)

38-202: LGTM!

Comprehensive integration tests covering:

  • Basic index operations with BytesValue
  • Update operations with PrimitiveFloatArray
  • Upsert operations with BytesValue
  • Bulk operations with mixed EFV types

The test structure is clean with appropriate assertions on stored field values.

server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraFieldValuesIngestTests.java (1)

29-135: LGTM!

Well-structured unit tests for EFV ingestion covering:

  • Root field with BytesValue
  • Nested object path with PrimitiveFloatArray
  • Error handling when no mapper exists for the extra field

The tests properly exercise the DocumentMapper.parse() path with SourceToParse containing ExtraFieldValues.

server/src/main/java/org/opensearch/index/mapper/extrasource/ExtraFieldValue.java (1)

17-90: LGTM!

Clean sealed interface design with:

  • Appropriate @ExperimentalApi annotation for the new feature
  • Efficient type lookup via byte id with O(1) array access
  • Well-defined wire encoding protocol: [typeId][body]
  • Good error handling for unknown type ids

The TODO for future types (double, int, long) is noted and the design accommodates extensibility.

server/src/main/java/org/opensearch/index/mapper/extrasource/PackedFloatArray.java (1)

35-165: LGTM!

Well-implemented packed float array with:

  • Efficient lazy decoding with proper volatile fields for thread-safety
  • Zero-copy optimization for BytesArray backed references
  • Correct little-endian decoding in decodeAt()
  • Robust validation with overflow protection using Math.multiplyExact
  • Proper bounds checking in get()

The documented concurrency behavior (multiple threads may transiently materialize but results are equivalent) is correctly implemented with Java's volatile memory model guarantees.

server/src/test/java/org/opensearch/index/mapper/extrasource/ExtraTestFieldMapper.java (1)

26-106: LGTM!

Appropriate test-only mapper implementation that:

  • Properly implements supportsExtraFieldValues() returning true
  • Consumes ExtraFieldValue via context.parseExternalValue()
  • Handles both BytesValue and FloatArrayValue types
  • Stores metadata fields for test verification (type, length/dimension, first float)

This provides the necessary test infrastructure for validating the EFV feature end-to-end.

server/src/test/java/org/opensearch/index/mapper/extrasource/PackedFloatArrayTests.java (1)

28-157: LGTM!

Comprehensive test coverage for PackedFloatArray including:

  • Basic construction and value retrieval
  • Zero-copy optimization verification for BytesArray slices
  • Composite BytesReference materialization behavior
  • Cache identity verification for asFloatArray()
  • Serialization round-trip
  • Validation and bounds checking error cases

The use of reflection to verify internal state is appropriate for testing optimization guarantees.

Comment on lines 724 to 733
/**
* Sets extral field values for the partial document update ({@code doc}).
* <p>
* These values are applied only when the update is executed using {@code doc} (i.e., no script).
* {@code null} clears the values and resets to {@link ExtraFieldValues#EMPTY}.
*/
public UpdateRequest docExtraFieldValues(ExtraFieldValues values) {
safeDoc().extraFieldValues(values);
return this;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix Javadoc typo ("extral").

📝 Proposed fix
-     * Sets extral field values for the partial document update ({`@code` doc}).
+     * Sets extra field values for the partial document update ({`@code` doc}).
🤖 Prompt for AI Agents
In `@server/src/main/java/org/opensearch/action/update/UpdateRequest.java` around
lines 724 - 733, The Javadoc for UpdateRequest.docExtraFieldValues has a typo
("extral") and should be corrected to "extra"; update the method comment block
above public UpdateRequest docExtraFieldValues(ExtraFieldValues values) to read
"Sets extra field values for the partial document update" and adjust any related
wording (e.g., "These values...") while keeping the rest of the documentation
and references to ExtraFieldValues, safeDoc().extraFieldValues(values), and
ExtraFieldValues#EMPTY unchanged.

@github-actions
Copy link
Contributor

❌ Gradle check result for 5d4649e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

use writeFloatArray/readFloatArray

Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
@github-actions
Copy link
Contributor

❌ Gradle check result for 573da92: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…rialization (version mismatch)

Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
@github-actions
Copy link
Contributor

❌ Gradle check result for 0f11c94: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@laminelam
Copy link
Contributor Author

Hi @msfroh

As per our conversation, this is the non grpc part of the contribution.

The contribution is split across multiple PRs to simplify review and maintain separation of concerns.

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.

1 participant